[xen-4.13-testing test] 175453: tolerable FAIL - PUSHED
flight 175453 xen-4.13-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/175453/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail in 175440 pass in 175453 test-amd64-i386-pair 11 xen-install/dst_host fail pass in 175440 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail pass in 175440 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 174675 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 174675 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 174675 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 174675 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 174675 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 174675 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 174675 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 174675 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 174675 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 174675 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 174675 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 174675 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail
[linux-linus test] 175451: regressions - FAIL
flight 175451 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/175451/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-ws16-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-coresched-amd64-xl 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-qemut-win7-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start fail REGR. vs. 173462 test-amd64-amd64-xl-vhd 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-qemuu-nested-amd 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-qemuu-nested-intel 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-pvhv2-amd 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-qemut-ws16-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemut-debianhvm-amd64 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-freebsd12-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-freebsd11-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-credit2 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-pair12 xen-boot/src_hostfail REGR. vs. 173462 test-amd64-amd64-xl-pvhv2-intel 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-win7-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-pair13 xen-boot/dst_hostfail REGR. vs. 173462 test-amd64-amd64-xl-shadow8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-raw 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-qcow2 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-pair 12 xen-boot/src_host fail REGR. vs. 173462 test-amd64-amd64-libvirt-pair 13 xen-boot/dst_host fail REGR. vs. 173462 test-arm64-arm64-xl 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-libvirt-raw 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-credit1 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-multivcpu 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-pvshim8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-examine 8 reboot fail REGR. vs. 173462 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 173462 test-arm64-arm64-xl-credit2 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-ovmf-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-debianhvm-amd64 8 xen-bootfail REGR. vs. 173462 test-armhf-armhf-xl-credit2 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-xl 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-examine 8 reboot fail REGR. vs. 173462 test-amd64-amd64-xl 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-credit1 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-libvirt 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-examine-uefi 8 reboot fail REGR. vs. 173462 test-amd64-amd64-examine-bios 8 reboot fail REGR. vs. 173462 test-amd64-amd64-examine 8 reboot fail REGR. vs. 173462 test-amd64-amd64-pygrub 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-vhd 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-seattle 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-libvirt-xsm 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-libvirt-qcow2 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-xl-multivcpu 8 xen-bootfail REGR. vs. 173462 test-armhf-armhf-xl-credit1 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-libvirt-raw 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-xl-arndale 8 xen-boot
Re: Porting Xen in raspberry pi4B
Hi Vipul, Great that you managed to setup a debugging environment. The logs look very promising: it looks like xenfb.c is handling events as expected. So it would apparently seem that xen-fbfront.c -> xenfb.c connection is working. So the next step is the xenfb.c -> ./ui/vnc.c is working. It could be that the pixels and mouse events arrive just fine in xenfb.c, but then there is an issue with exporting them to the vncserver implementation inside QEMU, which is ./ui/vnc.c. The interesting functions there are qemu_create_displaysurface, qemu_create_displaysurface_from, dpy_gfx_replace_surface, dpy_gfx_update, and dpy_gfx_check_format. Specifically dpy_gfx_update should cause VNC to render the new area. qemu_create_displaysurface_from let VNC use the xenfb buffer directly with VNC, rather than using a secondary buffer and memory copies. Interestingly, dpy_gfx_check_format should be used to check if it is appropriate to share the buffer (qemu_create_displaysurface_from) or not (qemu_create_displaysurface) but we don't call it. I think it would be good to add a call to dpy_gfx_check_format in xenfb_update where we call qemu_create_displaysurface_from and also add a printk. You can try to disable the buffer sharing by replacing qemu_create_displaysurface_from with qemu_create_displaysurface. You can also try to use another QEMU UI like SDL to see if the problem is specific to VNC only. Cheers, Stefano On Mon, 19 Dec 2022, Vipul Suneja wrote: > Hi Stefano, > > Thanks! > > I could prepare a patch for adding debug printf logs in xenfb.c & re-compile > QEMU in yocto image. Just for reference, I have included logs > in all the functions. > Attaching qemu log file, could see the entry & exit logs coming up for > "xenfb_handle_events" & "xenfb_map_fb" also after the host machine > boots up. Can you please further assist, which parameters has to be cross > checked or any other input as per logs? > > Thanks & Regards, > Vipul Kumar > > On Thu, Dec 15, 2022 at 4:17 AM Stefano Stabellini > wrote: > Hi Vipul, > > For QEMU you actually need to follow the Yocto build process to update > the QEMU binary. That is because QEMU is a userspace application with > lots of library dependencies so we cannot just do "make" with a > cross-compiler like in the case of Xen. > > So you need to make changes to QEMU and then add those changes as a > patch to the Yocto QEMU build recipe, or configure Yocto to your local > tree to build QEMU. I am not a Yocto expert and the Yocto community > would be a better place to ask for advice there. You can see from here > some instructions on how to build Xen using a local tree, see the usage > of EXTERNALSRC (note that this is *not* what you need: you need to build > QEMU with a local tree, not Xen. But I thought that the wikipage might > still be a starting point) > > https://wiki.xenproject.org/wiki/Xen_on_ARM_and_Yocto > > Cheers, > > Stefano > > > On Thu, 15 Dec 2022, Vipul Suneja wrote: > > Hi Stefano, > > > > Thanks! > > > > I could see QEMU 6.2.0 compiled & installed in the host image > xen-image-minimal. I could find xenfb.c source file also & > modified the same > > with debug logs. > > I have set up a cross compile environment, did 'make clean' & 'make > all' to recompile but it's failing. In case i am doing > wrong, Can you > > please assist me > > with the correct steps to compile qemu? Below are the error logs: > > > > > > agl@agl-OptiPlex-7010:~/Automotive/ADAS_Infotainment/Platform/Poky_Kirkstone/build/tmp/work/cortexa72-poky-linux/qemu/6.2.0-r0/build$ > make > > all > > [1/3864] Compiling C object libslirp.a.p/slirp_src_arp_table.c.o > > [2/3864] Compiling C object > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o > > [3/3864] Linking static target > subprojects/libvhost-user/libvhost-user.a > > [4/3864] Compiling C object libslirp.a.p/slirp_src_vmstate.c.o > > [5/3864] Compiling C object libslirp.a.p/slirp_src_dhcpv6.c.o > > [6/3864] Compiling C object libslirp.a.p/slirp_src_dnssearch.c.o > > [7/3864] Compiling C object libslirp.a.p/slirp_src_bootp.c.o > > [8/3864] Compiling C object libslirp.a.p/slirp_src_cksum.c.o > > [9/3864] Compiling C object libslirp.a.p/slirp_src_if.c.o > > [10/3864] Compiling C object libslirp.a.p/slirp_src_ip6_icmp.c.o > > [11/3864] Compiling C object libslirp.a.p/slirp_src_ip6_input.c.o > > [12/3864] Compiling C object libslirp.a.p/slirp_src_ip6_output.c.o > > [13/3864] Compiling C object libslirp.a.p/slirp_src_ip_icmp.c.o > > [14/3864] Compiling C object libslirp.a.p/slirp_src_ip_input.c.o > > [15/3864] Compiling C object libslirp.a.p/slirp_src_ip_output.c.o > > [16/3864] Compiling C object
Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t
On Sat, 17 Dec 2022, Julien Grall wrote: > On 17/12/2022 00:46, Stefano Stabellini wrote: > > On Fri, 16 Dec 2022, Julien Grall wrote: > > > Hi Ayan, > > > > > > On 15/12/2022 19:32, Ayan Kumar Halder wrote: > > > > paddr_t may be u64 or u32 depending of the type of architecture. > > > > Thus, while translating between u64 and paddr_t, one should check that > > > > the > > > > truncated bits are 0. If not, then raise an appropriate error. > > > > > > I am not entirely convinced this extra helper is worth it. If the user > > > can't > > > provide 32-bit address in the DT, then there are also a lot of other part > > > that > > > can go wrong. > > > > > > Bertrand, Stefano, what do you think? > > > > In general, it is not Xen's job to protect itself against bugs in device > > tree. However, if Xen spots a problem in DT and prints a helpful message > > that is better than just crashing because it gives a hint to the > > developer about what the problem is. > > I agree with the principle. In practice this needs to be weight out with the > long-term maintenance. > > > > > In this case, I think a BUG_ON would be sufficient. > > BUG_ON() is the same as panic(). They both should be used only when there are > no way to recover (see CODING_STYLE). > > If we parse the device-tree at boot, then BUG_ON() is ok. However, if we need > to parse it after boot (e.g. the DT overlay from Vikram), then we should > definitely not call BUG_ON() for checking DT input. yeah, I wasn't thinking of that series > The correct way is like Ayan did by checking returning an error and let > the caller deciding what to do. > > My concern with his approach is the extra amount of code in each caller to > check it (even with a new helper). > > I am not fully convinced that checking the addresses in the DT is that useful. I am also happy not to do it to be honest > However, if you both want to do it, then I think it would be best to introduce > wrappers over the Device-Tree ones that will check the truncation. > > For example, we could introduce dt_device_get_address_checked() > that will call dt_device_get_address() and then check for 32-bit truncation. > > For the function device_tree_get_reg(), this helper was aimed to deal with > just Physical address 'reg' very early. So we can modify the prototype and > update the function to check for 32-bit truncation. > > Note that I am suggesting a different approach for the two helpers because the > former is generic and it is not clear to me whether this could be used in > another context than physical address.
Re: [PATCH v1] xen/pvcalls: free active map buffer on pvcalls_front_free_map
On Tue, 20 Dec 2022, Oleksii Moisieiev wrote:
> Data buffer for active map is allocated in alloc_active_ring and freed
> in free_active_ring function, which is used only for the error
> cleanup. pvcalls_front_release is calling pvcalls_front_free_map which
> ends foreign access for this buffer, but doesn't free allocated pages.
> Call free_active_ring to clean all allocated resources.
>
> Signed-off-by: Oleksii Moisieiev
Reviewed-by: Stefano Stabellini
> ---
> drivers/xen/pvcalls-front.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index 1826e8e67125..9b569278788a 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -225,6 +225,8 @@ static irqreturn_t pvcalls_front_event_handler(int irq,
> void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void free_active_ring(struct sock_mapping *map);
> +
> static void pvcalls_front_free_map(struct pvcalls_bedata *bedata,
> struct sock_mapping *map)
> {
> @@ -240,7 +242,7 @@ static void pvcalls_front_free_map(struct pvcalls_bedata
> *bedata,
> for (i = 0; i < (1 << PVCALLS_RING_ORDER); i++)
> gnttab_end_foreign_access(map->active.ring->ref[i], NULL);
> gnttab_end_foreign_access(map->active.ref, NULL);
> - free_page((unsigned long)map->active.ring);
> + free_active_ring(map);
>
> kfree(map);
> }
> --
> 2.25.1
>
[PATCH v6 2/5] x86: Remove MEMORY_NUM_TYPES and NO_HARDCODE_MEM_TYPE
No functional change intended.
Signed-off-by: Demi Marie Obenour
---
Changes since v2:
- Keep MTRR_NUM_TYPES and adjust commit message accordingly
---
xen/arch/x86/hvm/mtrr.c | 18 +-
xen/arch/x86/include/asm/mtrr.h | 2 --
xen/arch/x86/mm/shadow/multi.c | 2 +-
3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index
093103f6c768cf64f880d1b20e1c14f5918c1250..05e978041d62fd0d559462de181a04bef8a5bca9
100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -38,7 +38,7 @@ static const uint8_t pat_entry_2_pte_flags[8] = {
/* Effective mm type lookup table, according to MTRR and PAT. */
static const uint8_t mm_type_tbl[MTRR_NUM_TYPES][X86_NUM_MT] = {
-#define RS MEMORY_NUM_TYPES
+#define RS MTRR_NUM_TYPES
#define UC X86_MT_UC
#define WB X86_MT_WB
#define WC X86_MT_WC
@@ -66,9 +66,9 @@ static const uint8_t mm_type_tbl[MTRR_NUM_TYPES][X86_NUM_MT]
= {
* Reverse lookup table, to find a pat type according to MTRR and effective
* memory type. This table is dynamically generated.
*/
-static uint8_t __read_mostly mtrr_epat_tbl[MTRR_NUM_TYPES][MEMORY_NUM_TYPES] =
-{ [0 ... MTRR_NUM_TYPES-1] =
-{ [0 ... MEMORY_NUM_TYPES-1] = INVALID_MEM_TYPE }
+static uint8_t __read_mostly mtrr_epat_tbl[MTRR_NUM_TYPES][MTRR_NUM_TYPES] =
+{ [0 ... MTRR_NUM_TYPES - 1] =
+{ [0 ... MTRR_NUM_TYPES - 1] = INVALID_MEM_TYPE }
};
/* Lookup table for PAT entry of a given PAT value in host PAT. */
@@ -85,7 +85,7 @@ static int __init cf_check hvm_mtrr_pat_init(void)
{
unsigned int tmp = mm_type_tbl[i][j];
-if ( tmp < MEMORY_NUM_TYPES )
+if ( tmp < MTRR_NUM_TYPES )
mtrr_epat_tbl[i][tmp] = j;
}
}
@@ -317,11 +317,11 @@ static uint8_t effective_mm_type(struct mtrr_state *m,
uint8_t gmtrr_mtype)
{
uint8_t mtrr_mtype, pat_value;
-
+
/* if get_pat_flags() gives a dedicated MTRR type,
* just use it
- */
-if ( gmtrr_mtype == NO_HARDCODE_MEM_TYPE )
+ */
+if ( gmtrr_mtype == MTRR_NUM_TYPES )
mtrr_mtype = mtrr_get_type(m, gpa, 0);
else
mtrr_mtype = gmtrr_mtype;
@@ -346,7 +346,7 @@ uint32_t get_pat_flags(struct vcpu *v,
/* 1. Get the effective memory type of guest physical address,
* with the pair of guest MTRR and PAT
*/
-guest_eff_mm_type = effective_mm_type(g, pat, gpaddr,
+guest_eff_mm_type = effective_mm_type(g, pat, gpaddr,
gl1e_flags, gmtrr_mtype);
/* 2. Get the memory type of host physical address, with MTRR */
shadow_mtrr_type = mtrr_get_type(_state, spaddr, 0);
diff --git a/xen/arch/x86/include/asm/mtrr.h b/xen/arch/x86/include/asm/mtrr.h
index
e4f6ca6048334b2094a1836cc2f298453641232f..4b7f840a965954cc4b59698327a37e47026893a4
100644
--- a/xen/arch/x86/include/asm/mtrr.h
+++ b/xen/arch/x86/include/asm/mtrr.h
@@ -4,8 +4,6 @@
#include
#define MTRR_NUM_TYPES X86_MT_UCM
-#define MEMORY_NUM_TYPES MTRR_NUM_TYPES
-#define NO_HARDCODE_MEM_TYPE MTRR_NUM_TYPES
#define NORMAL_CACHE_MODE 0
#define NO_FILL_CACHE_MODE 2
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index
f5f7ff021bd9e057c5b6f6329de7acb5ef05d58f..1faf9940db6b0afefc5977c00c00fb6a39cd27d2
100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -578,7 +578,7 @@ _sh_propagate(struct vcpu *v,
gflags,
gfn_to_paddr(target_gfn),
mfn_to_maddr(target_mfn),
-NO_HARDCODE_MEM_TYPE);
+MTRR_NUM_TYPES);
}
}
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[PATCH v6 4/5] x86/mm: Reject invalid cacheability in PV guests by default
Setting cacheability flags that are not ones specified by Xen is a bug
in the guest. By default, return -EINVAL if a guests attempts to do
this. The invalid-cacheability= Xen command-line flag allows the
administrator to allow such attempts or to produce
Suggested-by: Andrew Cooper
Signed-off-by: Demi Marie Obenour
---
Changes since v5:
- Make parameters static and __ro_after_init.
- Replace boolean parameter allow_invalid_cacheability with string
parameter invalid-cacheability.
- Move parameter definitions to near where they are used.
- Add documentation.
Changes since v4:
- Remove pointless BUILD_BUG_ON().
- Add comment explaining why an exception is being injected.
Changes since v3:
- Add Andrew Cooper’s Suggested-by
---
docs/misc/xen-command-line.pandoc | 11 ++
xen/arch/x86/mm.c | 60 ++-
2 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index
424b12cfb27d6ade2ec63eacb8afe5df82465451..0230a7bc17cbd4362a42ea64cea695f31f5e0f86
100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1417,6 +1417,17 @@ detection of systems known to misbehave upon accesses to
that port.
### idle_latency_factor (x86)
> `= `
+### invalid-cacheability (x86)
+> `= allow | deny | trap`
+
+> Default: `deny` in release builds, otherwise `trap`
+
+Specify what happens when a PV guest tries to use one of the reserved entries
in
+the PAT. `deny` causes the attempt to be rejected with -EINVAL, `allow` allows
+the attempt, and `trap` causes a general protection fault to be raised.
+Currently, the reserved entries are marked as uncacheable in Xen's PAT, but
this
+will change if new memory types are added, so guests must not rely on it.
+
### ioapic_ack (x86)
> `= old | new`
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index
65ba0f58ed8c26ac0343528303851739981c03bd..bacfb776d688f68dcbf79d83723fff329b75fd18
100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1324,6 +1324,37 @@ static int put_page_from_l4e(l4_pgentry_t l4e, mfn_t
l4mfn, unsigned int flags)
return put_pt_page(l4e_get_page(l4e), mfn_to_page(l4mfn), flags);
}
+enum {
+INVALID_CACHEABILITY_ALLOW,
+INVALID_CACHEABILITY_DENY,
+INVALID_CACHEABILITY_TRAP,
+};
+
+#ifdef NDEBUG
+#define INVALID_CACHEABILITY_DEFAULT INVALID_CACHEABILITY_DENY
+#else
+#define INVALID_CACHEABILITY_DEFAULT INVALID_CACHEABILITY_TRAP
+#endif
+
+static __ro_after_init uint8_t invalid_cacheability =
+INVALID_CACHEABILITY_DEFAULT;
+
+static int __init cf_check set_invalid_cacheability(const char *str)
+{
+if (strcmp("allow", str) == 0)
+invalid_cacheability = INVALID_CACHEABILITY_ALLOW;
+else if (strcmp("deny", str) == 0)
+invalid_cacheability = INVALID_CACHEABILITY_DENY;
+else if (strcmp("trap", str) == 0)
+invalid_cacheability = INVALID_CACHEABILITY_TRAP;
+else
+return -EINVAL;
+
+return 0;
+}
+
+custom_param("invalid-cacheability", set_invalid_cacheability);
+
static int promote_l1_table(struct page_info *page)
{
struct domain *d = page_get_owner(page);
@@ -1343,7 +1374,34 @@ static int promote_l1_table(struct page_info *page)
}
else
{
-switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
+l1_pgentry_t l1e = pl1e[i];
+
+if ( invalid_cacheability != INVALID_CACHEABILITY_ALLOW )
+{
+switch ( l1e.l1 & PAGE_CACHE_ATTRS )
+{
+case _PAGE_WB:
+case _PAGE_UC:
+case _PAGE_UCM:
+case _PAGE_WC:
+case _PAGE_WT:
+case _PAGE_WP:
+break;
+default:
+/*
+ * If we get here, a PV guest tried to use one of the
+ * reserved values in Xen's PAT. This indicates a bug
+ * in the guest. If requested by the user, inject #GP
+ * to cause the guest to log a stack trace.
+ */
+if ( invalid_cacheability == INVALID_CACHEABILITY_TRAP )
+pv_inject_hw_exception(TRAP_gp_fault, 0);
+ret = -EINVAL;
+goto fail;
+}
+}
+
+switch ( ret = get_page_from_l1e(l1e, d, d) )
{
default:
goto fail;
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[PATCH v6 3/5] x86/mm: make code robust to future PAT changes
It may be desirable to change Xen's PAT for various reasons. This
requires changes to several _PAGE_* macros as well. Add static
assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
macros, and that _PAGE_WB is 0 as required by Linux.
Signed-off-by: Demi Marie Obenour
---
Changes since v5:
- Remove unhelpful comment.
- Move a BUILD_BUG_ON.
- Use fewer hardcoded constants in PTE_FLAGS_TO_CACHEATTR.
- Fix coding style.
Changes since v4:
- Add lots of comments explaining what the various BUILD_BUG_ON()s mean.
Changes since v3:
- Refactor some macros
- Avoid including a string literal in BUILD_BUG_ON
---
xen/arch/x86/mm.c | 70 +++
1 file changed, 70 insertions(+)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index
3558ca215b02a517d55d75329d645ae5905424e4..65ba0f58ed8c26ac0343528303851739981c03bd
100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
}
+
+/*
+ * A bunch of static assertions to check that the XEN_MSR_PAT is valid
+ * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
+ */
static void __init __maybe_unused build_assertions(void)
{
/*
@@ -6361,6 +6366,71 @@ static void __init __maybe_unused build_assertions(void)
* using different PATs will not work.
*/
BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
+
+/*
+ * _PAGE_WB must be zero for several reasons, not least because Linux
+ * PV guests assume it.
+ */
+BUILD_BUG_ON(_PAGE_WB);
+
+#define PAT_ENTRY(v)
\
+(BUILD_BUG_ON_ZERO(((v) < 0) || ((v) > 7)) +
\
+ (0xFF & (XEN_MSR_PAT >> (8 * (v)
+
+/* Validate at compile-time that v is a valid value for a PAT entry */
+#define CHECK_PAT_ENTRY_VALUE(v)
\
+BUILD_BUG_ON((v) < 0 || (v) > 7 ||
\
+ (v) == X86_MT_RSVD_2 || (v) == X86_MT_RSVD_3)
+
+/* Validate at compile-time that PAT entry v is valid */
+#define CHECK_PAT_ENTRY(v) CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v))
+
+/*
+ * If one of these trips, the corresponding entry in XEN_MSR_PAT is
invalid.
+ * This would cause Xen to crash (with #GP) at startup.
+ */
+CHECK_PAT_ENTRY(0);
+CHECK_PAT_ENTRY(1);
+CHECK_PAT_ENTRY(2);
+CHECK_PAT_ENTRY(3);
+CHECK_PAT_ENTRY(4);
+CHECK_PAT_ENTRY(5);
+CHECK_PAT_ENTRY(6);
+CHECK_PAT_ENTRY(7);
+
+#undef CHECK_PAT_ENTRY
+#undef CHECK_PAT_ENTRY_VALUE
+
+/* Macro version of pte_flags_to_cacheattr(), for use in BUILD_BUG_ON()s */
+#define PTE_FLAGS_TO_CACHEATTR(pte_value)
\
+pte_value) & _PAGE_PAT) >> 5) |
\
+ (((pte_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3))
+
+/* Check that a PAT-related _PAGE_* macro is correct */
+#define CHECK_PAGE_VALUE(page_value) do {
\
+/* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */
\
+BUILD_BUG_ON(((_PAGE_ ## page_value) & PAGE_CACHE_ATTRS) !=
\
+ (_PAGE_ ## page_value));
\
+/* Check that the _PAGE_* are consistent with XEN_MSR_PAT */
\
+BUILD_BUG_ON(PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value)) !=
\
+ (X86_MT_ ## page_value));
\
+} while ( false )
+
+/*
+ * If one of these trips, the corresponding _PAGE_* macro is inconsistent
+ * with XEN_MSR_PAT. This would cause Xen to use incorrect cacheability
+ * flags, with results that are unknown and possibly harmful.
+ */
+CHECK_PAGE_VALUE(WT);
+CHECK_PAGE_VALUE(WB);
+CHECK_PAGE_VALUE(WC);
+CHECK_PAGE_VALUE(UC);
+CHECK_PAGE_VALUE(UCM);
+CHECK_PAGE_VALUE(WP);
+
+#undef CHECK_PAGE_VALUE
+#undef PAGE_FLAGS_TO_CACHEATTR
+#undef PAT_ENTRY
}
/*
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[PATCH v6 5/5] x86: Use Linux's PAT
This is purely for testing, to see if it works around a bug in i915. It
is not intended to be merged.
NOT-signed-off-by: DO NOT MERGE
---
xen/arch/x86/include/asm/page.h | 4 ++--
xen/arch/x86/include/asm/processor.h | 10 +-
xen/arch/x86/mm.c| 8
3 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/include/asm/page.h b/xen/arch/x86/include/asm/page.h
index
b585235d064a567082582c8e92a4e8283fd949ca..ab9b46f1d0901e50a83fd035ff28d1bda0b781a2
100644
--- a/xen/arch/x86/include/asm/page.h
+++ b/xen/arch/x86/include/asm/page.h
@@ -333,11 +333,11 @@ void efi_update_l4_pgtable(unsigned int l4idx,
l4_pgentry_t);
/* Memory types, encoded under Xen's choice of MSR_PAT. */
#define _PAGE_WB (0)
-#define _PAGE_WT (_PAGE_PWT)
+#define _PAGE_WC (_PAGE_PWT)
#define _PAGE_UCM(_PAGE_PCD)
#define _PAGE_UC (_PAGE_PCD | _PAGE_PWT)
-#define _PAGE_WC (_PAGE_PAT)
#define _PAGE_WP (_PAGE_PAT | _PAGE_PWT)
+#define _PAGE_WT (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)
/*
* Debug option: Ensure that granted mappings are not implicitly unmapped.
diff --git a/xen/arch/x86/include/asm/processor.h
b/xen/arch/x86/include/asm/processor.h
index
60b902060914584957db8afa5c7c1e6abdad4d13..3993d5638626f0948bb7ac8192d2eda187eb1bdb
100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -94,16 +94,16 @@
/*
* Host IA32_CR_PAT value to cover all memory types. This is not the default
- * MSR_PAT value, and is an ABI with PV guests.
+ * MSR_PAT value, and is needed by the Linux i915 driver.
*/
#define XEN_MSR_PAT ((_AC(X86_MT_WB, ULL) << 0x00) | \
- (_AC(X86_MT_WT, ULL) << 0x08) | \
+ (_AC(X86_MT_WC, ULL) << 0x08) | \
(_AC(X86_MT_UCM, ULL) << 0x10) | \
(_AC(X86_MT_UC, ULL) << 0x18) | \
- (_AC(X86_MT_WC, ULL) << 0x20) | \
+ (_AC(X86_MT_WB, ULL) << 0x20) | \
(_AC(X86_MT_WP, ULL) << 0x28) | \
- (_AC(X86_MT_UC, ULL) << 0x30) | \
- (_AC(X86_MT_UC, ULL) << 0x38))
+ (_AC(X86_MT_UCM, ULL) << 0x30) | \
+ (_AC(X86_MT_WT, ULL) << 0x38))
#ifndef __ASSEMBLY__
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index
bacfb776d688f68dcbf79d83723fff329b75fd18..ea8c69e66c48419031add2e02da0a8eb6af8e49a
100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6417,14 +6417,6 @@ unsigned long get_upper_mfn_bound(void)
*/
static void __init __maybe_unused build_assertions(void)
{
-/*
- * If this trips, any guests that blindly rely on the public API in xen.h
- * (instead of reading the PAT from Xen, as Linux 3.19+ does) will be
- * broken. Furthermore, live migration of PV guests between Xen versions
- * using different PATs will not work.
- */
-BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
-
/*
* _PAGE_WB must be zero for several reasons, not least because Linux
* PV guests assume it.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[PATCH v6 1/5] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()
get_page_from_l1e() relied on Xen's choice of PAT, which is brittle in
the face of future PAT changes. Instead, compute the actual cacheability
used by the CPU and switch on that, as this will work no matter what PAT
Xen uses.
No functional change intended. This code is itself questionable and may
be removed in the future, but removing it would be an observable
behavior change and so is out of scope for this patch series.
Signed-off-by: Demi Marie Obenour
Reviewed-by: Jan Beulich
---
Changes since v5:
- Make comment in get_page_from_l1e() future-proof.
- Explicitly state how known-uncacheable and potentially-cacheable types
are handled.
Changes since v4:
- Do not add new pte_flags_to_cacheability() helper, as this code may be
removed in the near future and so adding a new helper for it is a bad
idea.
- Do not BUG() in the event of an unexpected cacheability. This cannot
happen, but it is simpler to force such types to UC than to prove that
the BUG() is not reachable.
Changes since v3:
- Compute and use the actual cacheability as seen by the processor.
Changes since v2:
- Improve commit message.
---
xen/arch/x86/mm.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index
1bda1ba697b434b6c884f17e599aa9b6d3b3dd76..3558ca215b02a517d55d75329d645ae5905424e4
100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -959,14 +959,16 @@ get_page_from_l1e(
flip = _PAGE_RW;
}
-switch ( l1f & PAGE_CACHE_ATTRS )
+switch ( 0xFF & (XEN_MSR_PAT >> (8 * pte_flags_to_cacheattr(l1f))) )
{
-case 0: /* WB */
-flip |= _PAGE_PWT | _PAGE_PCD;
+case X86_MT_UC:
+case X86_MT_UCM:
+case X86_MT_WC:
+/* not cacheable, allow */
break;
-case _PAGE_PWT: /* WT */
-case _PAGE_PWT | _PAGE_PAT: /* WP */
-flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
+default:
+/* potentially cacheable, force to UC */
+flip |= ((l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC);
break;
}
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[PATCH v6 0/5] Make PAT handling less brittle
While working on Qubes OS Marek found out that there were some PAT hacks in the Linux i195 driver. I decided to make Xen use Linux’s PAT to see if it solved the graphics glitches that were observed; it did. This required a substantial amount of preliminary work that is useful even without using Linux’s PAT. Patches 1 through 4 are the preliminary work and I would like them to be accepted into upstream Xen. Patch 4 does break ABI by rejecting the unused PAT entries, but this will only impact buggy PV guests and can be disabled with a Xen command-line option. Patch 5 actually switches to Linux’s PAT and is NOT intended to be merged (at least for now) as it would at a minimum break migration of PV guests from hosts that do not have the patch. Only patches 4 and 5 actually change Xen’s observable behavior. Patch 1 is a prerequisite that removes the last place where Xen hard-codes its current PAT value. Patch 2 is strictly cleanup. Patch 3 makes changing the PAT much less error-prone, as problems with the PAT or with the associated _PAGE_* constants will be detected at compile time. Demi Marie Obenour (5): x86/mm: Avoid hard-coding PAT in get_page_from_l1e() x86: Remove MEMORY_NUM_TYPES and NO_HARDCODE_MEM_TYPE x86/mm: make code robust to future PAT changes x86/mm: Reject invalid cacheability in PV guests by default x86: Use Linux's PAT docs/misc/xen-command-line.pandoc| 11 ++ xen/arch/x86/hvm/mtrr.c | 18 ++-- xen/arch/x86/include/asm/mtrr.h | 2 - xen/arch/x86/include/asm/page.h | 4 +- xen/arch/x86/include/asm/processor.h | 10 +- xen/arch/x86/mm.c| 146 --- xen/arch/x86/mm/shadow/multi.c | 2 +- 7 files changed, 162 insertions(+), 31 deletions(-) -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
[ovmf test] 175461: all pass - PUSHED
flight 175461 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175461/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf d8d4abdff9096a69ff59d96ac4a8dd0e19e5cbcc baseline version: ovmf 538ac013d6a673842d780c88b7b3c21730260e8e Last test of basis 175458 2022-12-22 13:44:17 Z0 days Testing same since 175461 2022-12-22 18:12:21 Z0 days1 attempts People who touched revisions under test: Guo Dong jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 538ac013d6..d8d4abdff9 d8d4abdff9096a69ff59d96ac4a8dd0e19e5cbcc -> xen-tested-master
Re: [RFC PATCH 01/18] arm: cppcheck: misra rule 20.7 deviations for alternative.h
On Thu, 22 Dec 2022, Jan Beulich wrote: > On 22.12.2022 03:01, Stefano Stabellini wrote: > > What do you guys think? Nice automatic 20.7 scans for all patches and > > for staging, but with the undesirable false-positive comments, or > > no-20.7 scans but no comments in the tree? > > Anything automatic which then also bugs submitters about issues should > have as few false positives as possible. Otherwise people may quickly > get into the habit of ignoring such reports. You have a point. That said, it would be easy to spot false positives in new patches once we start from a clean slate, but then if we merge the patch we would also have to add a deviation comment for the false positive. All in all, maybe it is best to skip checking for 20.7 for now in gitlab-ci.
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
On Thu, 22 Dec 2022, Julien Grall wrote: > > What other hypervisors might or might not do should not be a factor in > > this discussion and it would be best to leave it aside. > > To be honest, Demi has a point. At the moment, VMF is a very niche use-case > (see more below). So you would end up to use less than 10% of the normal Xen > on Arm code. A lot of people will likely wonder why using Xen in this case? [...] > > From an AMD/Xilinx point of view, most of our customers using Xen in > > productions today don't use any hypercalls in one or more of their VMs. > This suggests a mix of guests are running (some using hypercalls and other > not). It would not be possible if you were using VMF. It is true that the current limitations are very restrictive. In embedded, we have a few pure static partitioning deployments where no hypercalls are required (Linux is using hypercalls today but it could do without), so maybe VMF could be enabled, but admittedly in those cases the main focus today is safety and fault tolerance, rather than confidential computing. > > Xen is great for these use-cases and it is rather common in embedded. > > It is certainly a different configuration from what most are come to > > expect from Xen on the server/desktop x86 side. There is no question > > that guests without hypercalls are important for Xen on ARM. > > > As a Xen community we have a long history and strong interest in making > > Xen more secure and also, more recently, safer (in the ISO 26262 > > safety-certification sense). The VMF work is very well aligned with both > > of these efforts and any additional burder to attackers is certainly > > good for Xen. > > I agree that we have a strong focus on making Xen more secure. However, we > also need to look at the use cases for it. As it stands, there will no: > - IOREQ use (don't think about emulating TPM) > - GICv3 ITS > - stage-1 SMMUv3 > - decoding of instructions when there is no syndrome > - hypercalls (including event channels) > - dom0 > > That's a lot of Xen features that can't be used. Effectively you will make Xen > more "secure" for a very few users. Among these, the main problems affecting AMD/Xilinx users today would be: - decoding of instructions - hypercalls, especially event channels Decoding of instructions would affect all our deployments. For hypercalls, even in static partitioning deployments, sometimes event channels are used for VM-to-VM notifications. > > Now the question is what changes are necessary and how to make them to > > the codebase. And if it turns out that some of the changes are not > > applicable or too complex to accept, the decision will be made purely > > from a code maintenance point of view and will have nothing to do with > > VMs making no hypercalls being unimportant (i.e. if we don't accept one > > or more patches is not going to have anything to do with the use-case > > being unimportant or what other hypervisors might or might not do). > I disagree, I think this is also about use cases. On the paper VMF look very > great, but so far it still has a big flaw (the TTBR can be changed) and it > would restrict a lot what you can do. We would need to be very clear in the commit messages and documentation that with the current version of VMF we do *not* achieve confidential computing and we do *not* offer protections comparable to AMD SEV. It is still possible for Xen to access guest data, it is just a bit harder. >From an implementation perspective, if we can find a way to implement it that would be easy to maintain, then it might still be worth it. It would probably take only a small amount of changes on top of the "Remove the directmap" series to make it so "map_domain_page" doesn't work anymore after boot. That might be worth exploring if you and Jackson agree? One thing that would make it much more widely applicable is your idea of hypercalls bounce buffers. VMF might work with hypercalls if the guest always uses the same buffer to pass hypercalls parameters to Xen. That one buffer could remain mapped in Xen for the lifetime of the VM and the VM would know to use it only to pass parameters to Xen.
[libvirt test] 175450: tolerable all pass - PUSHED
flight 175450 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/175450/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175436 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175436 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175436 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: libvirt d7d405664599772af6fe00d5a6f6256889617f9d baseline version: libvirt 4102acc608819b15658ca3e37ceca7c89efae16b Last test of basis 175436 2022-12-21 04:20:17 Z1 days Testing same since 175450 2022-12-22 04:20:19 Z0 days1 attempts People who touched revisions under test: Marek Marczykowski-Górecki Michal Privoznik jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-qcow2 pass test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw pass test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports,
[xen-4.17-testing test] 175447: tolerable FAIL - PUSHED
flight 175447 xen-4.17-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/175447/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175096 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175096 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175096 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175096 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175096 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175096 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175096 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175096 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175096 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175096 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175096 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175096 test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass version targeted for testing: xen c4972a4272690384b15d5706f2a833aed636895e baseline version: xen
[ovmf test] 175458: all pass - PUSHED
flight 175458 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175458/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 538ac013d6a673842d780c88b7b3c21730260e8e baseline version: ovmf ec87305f90d90096aac2a4d91e3f6556e2ecd6b9 Last test of basis 175452 2022-12-22 07:10:57 Z0 days Testing same since 175458 2022-12-22 13:44:17 Z0 days1 attempts People who touched revisions under test: Min M Xu Min Xu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git ec87305f90..538ac013d6 538ac013d6a673842d780c88b7b3c21730260e8e -> xen-tested-master
[xen-unstable test] 175446: FAIL
flight 175446 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/175446/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-migrupgrade broken in 175434 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm broken in 175434 test-amd64-i386-xl-qemut-debianhvm-i386-xsm broken in 175434 test-amd64-i386-xl-pvshimbroken in 175434 test-amd64-i386-libvirt-pair broken in 175434 Tests which are failing intermittently (not blocking): test-amd64-i386-xl-pvshim5 host-install(5) broken in 175434 pass in 175446 test-amd64-i386-libvirt-pair 6 host-install/src_host(6) broken in 175434 pass in 175446 test-amd64-i386-migrupgrade 6 host-install/src_host(6) broken in 175434 pass in 175446 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 5 host-install(5) broken in 175434 pass in 175446 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 5 host-install(5) broken in 175434 pass in 175446 test-amd64-amd64-examine-uefi 5 host-install broken in 175434 pass in 175446 test-amd64-i386-freebsd10-i386 7 xen-installfail in 175434 pass in 175446 test-amd64-amd64-examine-uefi 4 memdisk-try-append fail in 175434 pass in 175446 test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-installfail pass in 175434 test-amd64-amd64-qemuu-freebsd12-amd64 19 guest-localmigrate/x10 fail pass in 175434 test-amd64-i386-xl-vhd 21 guest-start/debian.repeat fail pass in 175434 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175399 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175399 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175399 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175399 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175399 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175399 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175399 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175399 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175399 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175399 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175399 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175399 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail
Re: [RFC PATCH] xen/common: cache colored buddy allocator for domains
Hi Jan On Wed, Dec 7, 2022 at 12:52 PM Jan Beulich wrote: > > On 22.10.2022 18:08, Carlo Nonato wrote: > > This commit replaces the colored allocator for domains with a simple buddy > > allocator indexed also by colors, so that it can allocate pages based on > > some coloring configuration. > > > > It applies on top of Arm cache coloring (v3) as sent to the mailing list. > > > > This has two benefits: > > - order can now be greater than 0 if the color config contains a > >sufficient number of adjacent colors starting from an order aligned > >one; > > But still not large enough to reach the order needed for large page > mappings, aiui? Yeah, but that's because it's difficult, AFAIK, to have a platform with that number of colors (e.g. level-2 mappings requires 512 adjacent colors, so a 32 MiB cache). Using large pages should be possible only when all colors are selected for a domain, but this implementation isn't that smart. The maximum order is determined only by the number of colors of the platform (e.g. 32 colors := order 5). Anyway the colored allocator is useless if a domain can use all colors, so in that situation I would switch to the normal buddy. > > - same benefits of the normal buddy: constant time alloc and free > >(constant with respect to the number of pages, not for the number of > >colors); > > > > But also one "big" cons: > > - given the way Xen queries the allocator, it can only serve larger pages > >first and only when a domain runs out of those, it can go with the > > smaller > >ones. Let's say that domain 0 has 31 colors out of 32 total (0-30 out of > >0-31). The order-4 pages (0-15) are allocated first and then the order-3 > >(16-23, since 0-7 and 8-15 are all already allocated), and then order-2 > >and so on. The result is... the domain practically uses only one half of > >the colors that it should. > > What's unclear to me is how big of a con this is, i.e. how reasonable it is > for someone to configure a domain to use all except one of the colors (and > not, say, half of them). Well that was just an extreme example, but many configurations are affected. Basically the best configuration is one that is "aligned" to a power of 2 (e.g. 0-3, 4-7, 16-31). Everything else that "mixes" powers of 2 (e.g. 0-5, 4-9, 16-18) will see this behavior where bigger chunks are preferred and it practically makes the domain use less of its cache partitions. The user could be warned about this in the docs, but it would also require extensive testing to see if there are any other drawbacks. > Jan Thanks. - Carlo Nonato
Re: [PATCH v3 4/9] tools/xl: add support for cache coloring configuration
Hi Anthony,
On Thu, Nov 24, 2022 at 4:21 PM Anthony PERARD
wrote:
>
> On Sat, Oct 22, 2022 at 05:51:15PM +0200, Carlo Nonato wrote:
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index b2901e04cf..5f53cec8bf 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -2880,6 +2880,16 @@ Currently, only the "sbsa_uart" model is supported
> > for ARM.
> >
> > =back
> >
> > +=over 4
> > +
> > +=item B
>
> Instead of COLORS_RANGE, maybe NUMBER_RANGE? Or just RANGE?
RANGE seems good.
> > +Specify the LLC color configuration for the guest. B can be
> > either
> > +a single color value or a hypen-separated closed interval of colors
> > +(such as "0-4").
>
> Does "yellow-blue" works? Or "red-violet", to have a rainbow? :-)
>
> So, "colors" as the name of a new configuration option isn't going to
> work. To me, that would refer to VM managment, like maybe help to
> categorise VM in some kind of tools, and not a part of the configuration
> of the domain.
>
> Could you invent a name that is more specific? Maybe "cache_colors" or
> something, or "vcpu_cache_colors".
What about llc_colors? LLC stands for Last Level Cache and I'm trying to use
it everywhere else since it's what is really implemented (not any cache is
colored, just the last level) and it's shorter than cache_colors.
> > +=back
> > +
> > =head3 x86
> >
> > =over 4
> > diff --git a/tools/libs/light/libxl_create.c
> > b/tools/libs/light/libxl_create.c
> > index b9dd2deedf..94c511912c 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -615,6 +615,7 @@ int libxl__domain_make(libxl__gc *gc,
> > libxl_domain_config *d_config,
> > struct xs_permissions rwperm[1];
> > struct xs_permissions noperm[1];
> > xs_transaction_t t = 0;
> > +DECLARE_HYPERCALL_BUFFER(unsigned int, colors);
> >
> > /* convenience aliases */
> > libxl_domain_create_info *info = _config->c_info;
> > @@ -676,6 +677,16 @@ int libxl__domain_make(libxl__gc *gc,
> > libxl_domain_config *d_config,
> > goto out;
> > }
> >
> > +if (d_config->b_info.num_colors) {
> > +size_t bytes = sizeof(unsigned int) *
> > d_config->b_info.num_colors;
> > +colors = xc_hypercall_buffer_alloc(ctx->xch, colors, bytes);
>
> Hypercall stuff is normally done in another library, libxenctrl (or
> maybe others like libxenguest). Is there a reason to do that here?
I moved this in xc_domain_create().
> > +memcpy(colors, d_config->b_info.colors, bytes);
> > +set_xen_guest_handle(create.arch.colors, colors);
> > +create.arch.num_colors = d_config->b_info.num_colors;
> > +create.arch.from_guest = 1;
>
> "arch" stuff is better dealt with in libxl__arch_domain_prepare_config().
> (unless it isn't arch specific in the next revision of the series)
Yes, removing arch specific parts is the current way of implementing it.
> > +LOG(DEBUG, "Setup %u domain colors",
> > d_config->b_info.num_colors);
>
>
>
> > +}
> > +
> > for (;;) {
> > uint32_t local_domid;
> > bool recent;
> > @@ -922,6 +933,7 @@ retry_transaction:
> > rc = 0;
> > out:
> > if (t) xs_transaction_end(ctx->xsh, t, 1);
> > +if (colors) xc_hypercall_buffer_free(ctx->xch, colors);
> > return rc;
> > }
> >
> > diff --git a/tools/libs/light/libxl_types.idl
> > b/tools/libs/light/libxl_types.idl
> > index d634f304cd..642173af1a 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -557,6 +557,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> > ("ioports", Array(libxl_ioport_range, "num_ioports")),
> > ("irqs", Array(uint32, "num_irqs")),
> > ("iomem",Array(libxl_iomem_range, "num_iomem")),
> > +("colors", Array(uint32, "num_colors")),
>
> So the colors is added to arch specific config in
> xen_domctl_createdomain, maybe we should do the same here and move it to
> the "arch_arm" struct. Or if that is declared common in hypervisor, then
> it is file to leave it here.
Yes, it will be declared in common.
> Also, "colors" needs to be rename to something more specific.
>
> > ("claim_mode",libxl_defbool),
> > ("event_channels", uint32),
> > ("kernel", string),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index 1b5381cef0..e6b2c7acff 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -1220,8 +1220,9 @@ void parse_config_data(const char *config_source,
> > XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
> > *usbctrls, *usbdevs, *p9devs, *vdispls,
> > *pvcallsifs_devs;
> > XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
> > - *mca_caps;
> > -int num_ioports, num_irqs, num_iomem,
[linux-5.4 test] 175445: FAIL
flight 175445 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/175445/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemut-debianhvm-amd64 broken in 175407 test-amd64-amd64-xl-credit2 broken in 175407 test-amd64-i386-qemuu-rhel6hvm-intel broken in 175407 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow broken in 175433 test-amd64-amd64-xl-qemuu-debianhvm-amd64 broken in 175433 test-amd64-amd64-xl broken in 175433 test-amd64-amd64-xl-qemuu-ovmf-amd64 broken in 175433 test-amd64-i386-pair broken in 175433 test-amd64-i386-xl-qemuu-ws16-amd64 broken in 175433 Tests which are failing intermittently (not blocking): test-amd64-i386-qemuu-rhel6hvm-intel 5 host-install(5) broken in 175407 pass in 175445 test-amd64-amd64-xl-credit2 5 host-install(5) broken in 175407 pass in 175445 test-amd64-amd64-xl-qemut-debianhvm-amd64 5 host-install(5) broken in 175407 pass in 175445 test-amd64-amd64-xl 5 host-install(5) broken in 175433 pass in 175445 test-amd64-amd64-xl-qemuu-debianhvm-amd64 5 host-install(5) broken in 175433 pass in 175445 test-amd64-amd64-xl-qemuu-ovmf-amd64 5 host-install(5) broken in 175433 pass in 175445 test-amd64-i386-xl-qemuu-ws16-amd64 5 host-install(5) broken in 175433 pass in 175445 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 5 host-install(5) broken in 175433 pass in 175445 test-amd64-i386-examine 5 host-install broken in 175433 pass in 175445 test-amd64-i386-pair 7 host-install/dst_host(7) broken in 175433 pass in 175445 test-armhf-armhf-xl-multivcpu 14 guest-start fail in 175407 pass in 175445 test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail in 175433 pass in 175407 test-armhf-armhf-xl-rtds 14 guest-start fail in 175433 pass in 175445 test-arm64-arm64-examine 8 reboot fail pass in 175433 test-armhf-armhf-xl 18 guest-start/debian.repeat fail pass in 175433 test-armhf-armhf-xl-credit1 14 guest-startfail pass in 175433 test-armhf-armhf-xl-vhd 13 guest-startfail pass in 175433 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail blocked in 175197 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail in 175407 like 175197 test-armhf-armhf-xl-vhd 14 migrate-support-check fail in 175433 never pass test-armhf-armhf-xl-vhd 15 saverestore-support-check fail in 175433 never pass test-armhf-armhf-xl-credit1 15 migrate-support-check fail in 175433 never pass test-armhf-armhf-xl-credit1 16 saverestore-support-check fail in 175433 never pass test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175197 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175197 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeatfail like 175197 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175197 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175197 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeatfail like 175197 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeatfail like 175197 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175197 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175197 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175197 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175197 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175197 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175197 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175197 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm
Re: [PATCH 12/22] xen/arm: fixmap: Rename the fixmap slots to follow the x86 convention
On 16.12.2022 12:48, Julien Grall wrote: > From: Julien Grall > > At the moment the fixmap slots are prefixed differently between arm and > x86. > > Some of them (e.g. the PMAP slots) are used in common code. So it would > be better if they are named the same way to avoid having to create > aliases. > > I have decided to use the x86 naming because they are less change. So > all the Arm fixmap slots will now be prefixed with FIX rather than > FIXMAP. > > Signed-off-by: Julien Grall Reviewed-by: Jan Beulich
Re: [PATCH 11/22] x86: add a boot option to enable and disable the direct map
On 16.12.2022 12:48, Julien Grall wrote: > From: Hongyan Xia > > Also add a helper function to retrieve it. Change arch_mfns_in_direct_map > to check this option before returning. I think the abstract parts of this want to be generic right away. I can't see why Arm would not suffer from the same issue that this work is trying to address. > This is added as a boot command line option, not a Kconfig to allow > the user to experiment the feature without rebuild the hypervisor. I think there wants to be a (generic) Kconfig piece here, to control the default of the option. Plus a 2nd, prompt-less element which an arch can select to force the setting to always-on, suppressing the choice of default. That 2nd control would then be used to compile out the boolean_param() for Arm for the time being. That said, I think this change comes too early in the series, or there is something missing. As said in reply to patch 10, while there the mapcache is being initialized for the idle domain, I don't think it can be used just yet. Read through mapcache_current_vcpu() to understand why I think that way, paying particular attention to the ASSERT() near the end. In preparation of this patch here I think the mfn_to_virt() uses have to all disappear from map_domain_page(). Perhaps yet more strongly all ..._to_virt() (except fix_to_virt() and friends) and __va() have to disappear up front from x86 and any code path which can be taken on x86 (which may simply mean purging all respective x86 #define-s, without breaking the build in any way). Jan
Re: [PATCH 10/22] x86/mapcache: initialise the mapcache for the idle domain
On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia
>
> In order to use the mapcache in the idle domain, we also have to
> populate its page tables in the PERDOMAIN region, and we need to move
> mapcache_domain_init() earlier in arch_domain_create().
>
> Note, commit 'x86: lift mapcache variable to the arch level' has
> initialised the mapcache for HVM domains. With this patch, PV, HVM,
> idle domains now all initialise the mapcache.
But they can't use it yet, can they? This needs saying explicitly, or
else one is going to make wrong implications.
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -732,6 +732,8 @@ int arch_domain_create(struct domain *d,
>
> spin_lock_init(>arch.e820_lock);
>
> +mapcache_domain_init(d);
> +
> /* Minimal initialisation for the idle domain. */
> if ( unlikely(is_idle_domain(d)) )
> {
> @@ -829,8 +831,6 @@ int arch_domain_create(struct domain *d,
>
> psr_domain_init(d);
>
> -mapcache_domain_init(d);
You move this ahead of error paths taking the "goto out" route, so
adjustments to affected error paths are going to be needed to avoid
memory leaks.
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5963,6 +5963,9 @@ int create_perdomain_mapping(struct domain *d, unsigned
> long va,
> l3tab = __map_domain_page(pg);
> clear_page(l3tab);
> d->arch.perdomain_l3_pg = pg;
> +if ( is_idle_domain(d) )
> +idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
> +l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
Hmm, having an idle domain check here isn't very nice. I agree putting
it in arch_domain_create()'s respective conditional isn't very neat
either, but personally I'd consider this at least a little less bad.
And the layering violation aspect isn't much worse than that of setting
d->arch.ctxt_switch there as well.
Jan
Re: [PATCH 09/22] x86: lift mapcache variable to the arch level
On 16.12.2022 12:48, Julien Grall wrote: > From: Wei Liu > > It is going to be needed by HVM and idle domain as well, because without > the direct map, both need a mapcache to map pages. > > This only lifts the mapcache variable up. Whether we populate the > mapcache for a domain is unchanged in this patch. > > Signed-off-by: Wei Liu > Signed-off-by: Wei Wang > Signed-off-by: Hongyan Xia > Signed-off-by: Julien Grall Reviewed-by: Jan Beulich
[xen-unstable-smoke test] 175455: tolerable all pass - PUSHED
flight 175455 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/175455/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass version targeted for testing: xen 43b5d7b14c569e2deaf6a2863cfa44351061ad80 baseline version: xen 9c57a297378932249c3edefa5065c838f47cb3fb Last test of basis 175449 2022-12-22 02:00:30 Z0 days Testing same since 175455 2022-12-22 10:00:28 Z0 days1 attempts People who touched revisions under test: Jan Beulich Luca Fancellu jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 9c57a29737..43b5d7b14c 43b5d7b14c569e2deaf6a2863cfa44351061ad80 -> smoke
Re: [PATCH 08/22] x86/pv: rewrite how building PV dom0 handles domheap mappings
On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia
>
> Building a PV dom0 is allocating from the domheap but uses it like the
> xenheap. This is clearly wrong. Fix.
"Clearly wrong" would mean there's a bug here, at lest under certain
conditions. But there isn't: Even on huge systems, due to running on
idle page tables, all memory is mapped at present.
> @@ -711,22 +715,32 @@ int __init dom0_construct_pv(struct domain *d,
> v->arch.pv.event_callback_cs= FLAT_COMPAT_KERNEL_CS;
> }
>
> +#define UNMAP_MAP_AND_ADVANCE(mfn_var, virt_var, maddr) \
> +do {\
> +UNMAP_DOMAIN_PAGE(virt_var);\
Not much point using the macro when ...
> +mfn_var = maddr_to_mfn(maddr); \
> +maddr += PAGE_SIZE; \
> +virt_var = map_domain_page(mfn_var);\
... the variable gets reset again to non-NULL unconditionally right
away.
> +} while ( false )
This being a local macro and all use sites passing mpt_alloc as the
last argument, I think that parameter wants dropping, which would
improve readability.
> @@ -792,9 +808,9 @@ int __init dom0_construct_pv(struct domain *d,
> if ( !l3e_get_intpte(*l3tab) )
> {
> maddr_to_page(mpt_alloc)->u.inuse.type_info =
> PGT_l2_page_table;
> -l2tab = __va(mpt_alloc); mpt_alloc += PAGE_SIZE;
> -clear_page(l2tab);
> -*l3tab = l3e_from_paddr(__pa(l2tab), L3_PROT);
> +UNMAP_MAP_AND_ADVANCE(l2start_mfn, l2start, mpt_alloc);
> +clear_page(l2start);
> +*l3tab = l3e_from_mfn(l2start_mfn, L3_PROT);
The l2start you map on the last iteration here can be re-used ...
> @@ -805,9 +821,17 @@ int __init dom0_construct_pv(struct domain *d,
> unmap_domain_page(l2t);
> }
... in the code the tail of which is visible here, eliminating a
redundant map/unmap pair.
> @@ -977,8 +1001,12 @@ int __init dom0_construct_pv(struct domain *d,
> * !CONFIG_VIDEO case so the logic here can be simplified.
> */
> if ( pv_shim )
> +{
> +l4start = map_domain_page(l4start_mfn);
> pv_shim_setup_dom(d, l4start, v_start, vxenstore_start,
> vconsole_start,
>vphysmap_start, si);
> +UNMAP_DOMAIN_PAGE(l4start);
> +}
The, at the first glance, redundant re-mapping of the L4 table here could
do with explaining in the description. However, I further wonder in how
far in shim mode eliminating the direct map is actually useful. Which is
to say that I question the need for this change in the first place. Or
wait - isn't this (unlike the rest of this patch) actually a bug fix? At
this point we're on the domain's page tables, which may not cover the
page the L4 is allocated at (if a truly huge shim was configured). So I
guess the change is needed but wants breaking out, allowing to at least
consider whether to backport it.
Jan
[qemu-mainline test] 175443: tolerable FAIL - PUSHED
flight 175443 qemu-mainline real [real] flight 175456 qemu-mainline real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/175443/ http://logs.test-lab.xenproject.org/osstest/logs/175456/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-install fail pass in 175456-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175394 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175394 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175394 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175394 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeatfail like 175394 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175394 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175394 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175394 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175394 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: qemuu8540a1f69578afb3b37866b1ce5bec46a9f6efbc baseline version: qemuu
Re: [PATCH 07/22] x86/pv: domheap pages should be mapped while relocating initrd
On 16.12.2022 12:48, Julien Grall wrote:
> From: Wei Liu
>
> Xen shouldn't use domheap page as if they were xenheap pages. Map and
> unmap pages accordingly.
>
> Signed-off-by: Wei Liu
> Signed-off-by: Wei Wang
> Signed-off-by: Julien Grall
>
>
>
> Changes since Hongyan's version:
> * Add missing newline after the variable declaration
> ---
> xen/arch/x86/pv/dom0_build.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index a62f0fa2ef29..c837b2d96f89 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -611,18 +611,32 @@ int __init dom0_construct_pv(struct domain *d,
> if ( d->arch.physaddr_bitsize &&
> ((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
> {
> +unsigned long nr_pages;
> +unsigned long len = initrd_len;
> +
> order = get_order_from_pages(count);
> page = alloc_domheap_pages(d, order, MEMF_no_scrub);
> if ( !page )
> panic("Not enough RAM for domain 0 initrd\n");
> +
> +nr_pages = 1UL << order;
I don't think this needs establishing here and ...
> for ( count = -count; order--; )
> if ( count & (1UL << order) )
> {
> free_domheap_pages(page, order);
> page += 1UL << order;
> +nr_pages -= 1UL << order;
... updating here. Doing so just once ...
> }
> -memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
> - initrd_len);
> +
... here ought to suffice, assuming this 2nd variable is needed at all
(alongside "len").
> +for ( i = 0; i < nr_pages; i++, len -= PAGE_SIZE )
> +{
> +void *p = __map_domain_page(page + i);
> +
> +memcpy(p, mfn_to_virt(initrd_mfn + i),
> + min(len, (unsigned long)PAGE_SIZE));
> +unmap_domain_page(p);
> +}
You're half open-coding copy_domain_page() without saying anywhere why
the remaining mfn_to_virt() is okay to keep. If you used
copy_domain_page(), no such remark would need adding in the description.
Jan
Re: [PATCH 06/22] x86: map/unmap pages in restore_all_guests
On 16.12.2022 12:48, Julien Grall wrote: > --- a/xen/arch/x86/x86_64/entry.S > +++ b/xen/arch/x86/x86_64/entry.S > @@ -165,7 +165,24 @@ restore_all_guest: > and %rsi, %rdi > and %r9, %rsi > add %rcx, %rdi > -add %rcx, %rsi > + > + /* > + * Without a direct map, we have to map first before copying. We > only > + * need to map the guest root table but not the per-CPU root_pgt, > + * because the latter is still a xenheap page. > + */ > +pushq %r9 > +pushq %rdx > +pushq %rax > +pushq %rdi > +mov %rsi, %rdi > +shr $PAGE_SHIFT, %rdi > +callq map_domain_page > +mov %rax, %rsi > +popq %rdi > +/* Stash the pointer for unmapping later. */ > +pushq %rax > + > mov $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx > mov root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8 > mov %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi) > @@ -177,6 +194,14 @@ restore_all_guest: > sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \ > ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi > rep movsq > + > +/* Unmap the page. */ > +popq %rdi > +callq unmap_domain_page > +popq %rax > +popq %rdx > +popq %r9 While the PUSH/POP are part of what I dislike here, I think this wants doing differently: Establish a mapping when putting in place a new guest page table, and use the pointer here. This could be a new per-domain mapping, to limit its visibility. Jan
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
On Thu, Dec 22, 2022 at 10:21:57AM +, Julien Grall wrote: > > > On 22/12/2022 10:14, Demi Marie Obenour wrote: > > On Thu, Dec 22, 2022 at 09:52:11AM +, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 22/12/2022 00:38, Stefano Stabellini wrote: > > > > On Tue, 20 Dec 2022, Smith, Jackson wrote: > > > > > > Hi Stefano, > > > > > > > > > > > > On 16/12/2022 01:46, Stefano Stabellini wrote: > > > > > > > On Thu, 15 Dec 2022, Julien Grall wrote: > > > > > > > > > > On 13/12/2022 19:48, Smith, Jackson wrote: > > > > > > > > > Yes, we are familiar with the "secret-free hypervisor" work. > > > > > > > > > As > > > > > you > > > > > > > > > point out, both our work and the secret-free hypervisor > > > > > > > > > remove the > > > > > > > > > directmap region to mitigate the risk of leaking sensitive > > > > > > > > > guest > > > > > > > > > secrets. However, our work is slightly different because it > > > > > > > > > additionally prevents attackers from tricking Xen into > > > > > > > > > remapping a > > > > > > guest. > > > > > > > > > > > > > > > > I understand your goal, but I don't think this is achieved (see > > > > > > > > above). You would need an entity to prevent write to TTBR0_EL2 > > > > > > > > in > > > > > > > > order to fully protect it. > > > > > > > > > > > > > > Without a way to stop Xen from reading/writing TTBR0_EL2, we > > > > > > cannot > > > > > > > claim that the guest's secrets are 100% safe. > > > > > > > > > > > > > > But the attacker would have to follow the sequence you outlines > > > > > > above > > > > > > > to change Xen's pagetables and remap guest memory before > > > > > > accessing it. > > > > > > > It is an additional obstacle for attackers that want to steal > > > > > > > other > > > > > > guests' > > > > > > > secrets. The size of the code that the attacker would need to > > > > > > > inject > > > > > > > in Xen would need to be bigger and more complex. > > > > > > > > > > > > Right, that's why I wrote with a bit more work. However, the nuance > > > > > > you mention doesn't seem to be present in the cover letter: > > > > > > > > > > > > "This creates what we call "Software Enclaves", ensuring that an > > > > > > adversary with arbitrary code execution in the hypervisor STILL > > > > > > cannot > > > > > > read/write guest memory." > > > > > > > > > > > > So if the end goal if really to protect against *all* sort of > > > > > arbitrary > > > > > > code, > > > > > > then I think we should have a rough idea how this will look like in > > > > > Xen. > > > > > > > > > > > >From a brief look, it doesn't look like it would be possible to > > > > > prevent > > > > > > modification to TTBR0_EL2 (even from EL3). We would need to > > > > > > investigate if there are other bits in the architecture to help us. > > > > > > > > > > > > > > > > > > > > Every little helps :-) > > > > > > > > > > > > I can see how making the life of the attacker more difficult is > > > > > > appealing. > > > > > > Yet, the goal needs to be clarified and the risk with the approach > > > > > > acknowledged (see above). > > > > > > > > > > > > > > > > You're right, we should have mentioned this weakness in our first > > > > > email. > > > > > Sorry about the oversight! This is definitely still a limitation that > > > > > we > > > > > have not yet overcome. However, we do think that the increase in > > > > > attacker workload that you and Stefano are discussing could still be > > > > > valuable to security conscious Xen users. > > > > > > > > > > It would nice to find additional architecture features that we can use > > > > > to close this hole on arm, but there aren't any that stand out to me > > > > > either. > > > > > > > > > > With this limitation in mind, what are the next steps we should take > > > > > to > > > > > support this feature for the xen community? Is this increase in > > > > > attacker > > > > > workload meaningful enough to justify the inclusion of VMF in Xen? > > > > > > > > I think it could be valuable as an additional obstacle for the attacker > > > > to overcome. The next step would be to port your series on top of > > > > Julien's "Remove the directmap" patch series > > > > https://marc.info/?l=xen-devel=167119090721116 > > > > > > > > Julien, what do you think? > > > > > > If we want Xen to be used in confidential compute, then we need a > > > compelling > > > story and prove that we are at least as secure as other hypervisors. > > > > > > So I think we need to investigate a few areas: > > > * Can we protect the TTBR? I don't think this can be done with the HW. > > > But maybe I overlook it. > > > > This can be done by running most of Xen at a lower EL, and having only a > > small trusted (and hopefully formally verified) kernel run at EL2. > > This is what I hinted in my 3rd bullet. :) I didn't consider this for the > first bullet because the goal of this question is to figure out whether we > can leave all Xen running in EL2 and
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
On 22/12/2022 10:14, Demi Marie Obenour wrote: On Thu, Dec 22, 2022 at 09:52:11AM +, Julien Grall wrote: Hi Stefano, On 22/12/2022 00:38, Stefano Stabellini wrote: On Tue, 20 Dec 2022, Smith, Jackson wrote: Hi Stefano, On 16/12/2022 01:46, Stefano Stabellini wrote: On Thu, 15 Dec 2022, Julien Grall wrote: On 13/12/2022 19:48, Smith, Jackson wrote: Yes, we are familiar with the "secret-free hypervisor" work. As you point out, both our work and the secret-free hypervisor remove the directmap region to mitigate the risk of leaking sensitive guest secrets. However, our work is slightly different because it additionally prevents attackers from tricking Xen into remapping a guest. I understand your goal, but I don't think this is achieved (see above). You would need an entity to prevent write to TTBR0_EL2 in order to fully protect it. Without a way to stop Xen from reading/writing TTBR0_EL2, we cannot claim that the guest's secrets are 100% safe. But the attacker would have to follow the sequence you outlines above to change Xen's pagetables and remap guest memory before accessing it. It is an additional obstacle for attackers that want to steal other guests' secrets. The size of the code that the attacker would need to inject in Xen would need to be bigger and more complex. Right, that's why I wrote with a bit more work. However, the nuance you mention doesn't seem to be present in the cover letter: "This creates what we call "Software Enclaves", ensuring that an adversary with arbitrary code execution in the hypervisor STILL cannot read/write guest memory." So if the end goal if really to protect against *all* sort of arbitrary code, then I think we should have a rough idea how this will look like in Xen. From a brief look, it doesn't look like it would be possible to prevent modification to TTBR0_EL2 (even from EL3). We would need to investigate if there are other bits in the architecture to help us. Every little helps :-) I can see how making the life of the attacker more difficult is appealing. Yet, the goal needs to be clarified and the risk with the approach acknowledged (see above). You're right, we should have mentioned this weakness in our first email. Sorry about the oversight! This is definitely still a limitation that we have not yet overcome. However, we do think that the increase in attacker workload that you and Stefano are discussing could still be valuable to security conscious Xen users. It would nice to find additional architecture features that we can use to close this hole on arm, but there aren't any that stand out to me either. With this limitation in mind, what are the next steps we should take to support this feature for the xen community? Is this increase in attacker workload meaningful enough to justify the inclusion of VMF in Xen? I think it could be valuable as an additional obstacle for the attacker to overcome. The next step would be to port your series on top of Julien's "Remove the directmap" patch series https://marc.info/?l=xen-devel=167119090721116 Julien, what do you think? If we want Xen to be used in confidential compute, then we need a compelling story and prove that we are at least as secure as other hypervisors. So I think we need to investigate a few areas: * Can we protect the TTBR? I don't think this can be done with the HW. But maybe I overlook it. This can be done by running most of Xen at a lower EL, and having only a small trusted (and hopefully formally verified) kernel run at EL2. This is what I hinted in my 3rd bullet. :) I didn't consider this for the first bullet because the goal of this question is to figure out whether we can leave all Xen running in EL2 and still have the same guarantee. Cheers, -- Julien Grall
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
On Thu, Dec 22, 2022 at 09:52:11AM +, Julien Grall wrote: > Hi Stefano, > > On 22/12/2022 00:38, Stefano Stabellini wrote: > > On Tue, 20 Dec 2022, Smith, Jackson wrote: > > > > Hi Stefano, > > > > > > > > On 16/12/2022 01:46, Stefano Stabellini wrote: > > > > > On Thu, 15 Dec 2022, Julien Grall wrote: > > > > > > > > On 13/12/2022 19:48, Smith, Jackson wrote: > > > > > > > Yes, we are familiar with the "secret-free hypervisor" work. As > > > you > > > > > > > point out, both our work and the secret-free hypervisor remove the > > > > > > > directmap region to mitigate the risk of leaking sensitive guest > > > > > > > secrets. However, our work is slightly different because it > > > > > > > additionally prevents attackers from tricking Xen into remapping a > > > > guest. > > > > > > > > > > > > I understand your goal, but I don't think this is achieved (see > > > > > > above). You would need an entity to prevent write to TTBR0_EL2 in > > > > > > order to fully protect it. > > > > > > > > > > Without a way to stop Xen from reading/writing TTBR0_EL2, we > > > > cannot > > > > > claim that the guest's secrets are 100% safe. > > > > > > > > > > But the attacker would have to follow the sequence you outlines > > > > above > > > > > to change Xen's pagetables and remap guest memory before > > > > accessing it. > > > > > It is an additional obstacle for attackers that want to steal other > > > > guests' > > > > > secrets. The size of the code that the attacker would need to inject > > > > > in Xen would need to be bigger and more complex. > > > > > > > > Right, that's why I wrote with a bit more work. However, the nuance > > > > you mention doesn't seem to be present in the cover letter: > > > > > > > > "This creates what we call "Software Enclaves", ensuring that an > > > > adversary with arbitrary code execution in the hypervisor STILL cannot > > > > read/write guest memory." > > > > > > > > So if the end goal if really to protect against *all* sort of > > > arbitrary > > > > code, > > > > then I think we should have a rough idea how this will look like in > > > Xen. > > > > > > > > From a brief look, it doesn't look like it would be possible to > > > prevent > > > > modification to TTBR0_EL2 (even from EL3). We would need to > > > > investigate if there are other bits in the architecture to help us. > > > > > > > > > > > > > > Every little helps :-) > > > > > > > > I can see how making the life of the attacker more difficult is > > > > appealing. > > > > Yet, the goal needs to be clarified and the risk with the approach > > > > acknowledged (see above). > > > > > > > > > > You're right, we should have mentioned this weakness in our first email. > > > Sorry about the oversight! This is definitely still a limitation that we > > > have not yet overcome. However, we do think that the increase in > > > attacker workload that you and Stefano are discussing could still be > > > valuable to security conscious Xen users. > > > > > > It would nice to find additional architecture features that we can use > > > to close this hole on arm, but there aren't any that stand out to me > > > either. > > > > > > With this limitation in mind, what are the next steps we should take to > > > support this feature for the xen community? Is this increase in attacker > > > workload meaningful enough to justify the inclusion of VMF in Xen? > > > > I think it could be valuable as an additional obstacle for the attacker > > to overcome. The next step would be to port your series on top of > > Julien's "Remove the directmap" patch series > > https://marc.info/?l=xen-devel=167119090721116 > > > > Julien, what do you think? > > If we want Xen to be used in confidential compute, then we need a compelling > story and prove that we are at least as secure as other hypervisors. > > So I think we need to investigate a few areas: >* Can we protect the TTBR? I don't think this can be done with the HW. > But maybe I overlook it. This can be done by running most of Xen at a lower EL, and having only a small trusted (and hopefully formally verified) kernel run at EL2. >* Can VMF be extended to more use-cases? For instances, for hypercalls, > we could have bounce buffer. >* If we can't fully secure VMF, can the attack surface be reduced (e.g. > disable hypercalls at runtime/compile time)? Could we use a different > architecture (I am thinking something like pKVM [1])? > > Cheers, > > [1] https://lwn.net/Articles/836693/ pKVM has been formally verified already, in the form of seKVM. So there very much is precident for this. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab signature.asc Description: PGP signature
Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default
On 22.12.2022 10:55, Demi Marie Obenour wrote: > On Thu, Dec 22, 2022 at 10:48:02AM +0100, Jan Beulich wrote: >> Also wasn't there talk of having this behavior in debug hypervisors >> only? Without that restriction I'm yet less happy with the change ... > > My understanding was that Andrew preferred the behavior to be turned on > in release hypervisors too. I am inclined to agree with Andrew, if for > no other reason than that those working on guest OSs do not generally > use debug hypervisors if they are not also working on Xen itself. That's likely a mistake then, at least for work on PV guest OSes. In particular PV memory management code is sprinkled with gdprintk(), in an attempt to help those people understand why some operation has failed. Jan
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On 22.12.2022 11:00, Demi Marie Obenour wrote: > On Thu, Dec 22, 2022 at 10:54:48AM +0100, Jan Beulich wrote: >> On 22.12.2022 10:50, Demi Marie Obenour wrote: >>> On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote: On 20.12.2022 02:07, Demi Marie Obenour wrote: > @@ -6361,6 +6366,72 @@ static void __init __maybe_unused > build_assertions(void) > * using different PATs will not work. > */ > BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL); > + > +/* > + * _PAGE_WB must be zero for several reasons, not least because Linux > + * assumes it. > + */ > +BUILD_BUG_ON(_PAGE_WB); Strictly speaking this is a requirement only for PV guests. We may not want to go as far as putting "#ifdef CONFIG_PV" around it, but at least the code comment (and then also the part of the description referring to this) will imo want to say so. >>> >>> Does Xen itself depend on this? >> >> With the wording in the description I was going from the assumption that >> you have audited code and found that we properly use _PAGE_* constants >> everywhere. > > There could be other hard-coded uses of magic numbers I haven’t found, > and _PAGE_WB is currently zero so I would be quite surpised if no code > in Xen omits it. Linux also has a BUILD_BUG_ON() to check the same > thing. Fair enough - please adjust description and comment text accordingly then. Jan
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On Thu, Dec 22, 2022 at 10:54:48AM +0100, Jan Beulich wrote:
> On 22.12.2022 10:50, Demi Marie Obenour wrote:
> > On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
> >> On 20.12.2022 02:07, Demi Marie Obenour wrote:
> >>> --- a/xen/arch/x86/mm.c
> >>> +++ b/xen/arch/x86/mm.c
> >>> @@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
> >>> return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
> >>> }
> >>>
> >>> +
> >>> +/*
> >>> + * A bunch of static assertions to check that the XEN_MSR_PAT is valid
> >>> + * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
> >>> + */
> >>> static void __init __maybe_unused build_assertions(void)
> >>> {
> >>> /*
> >>> @@ -6361,6 +6366,72 @@ static void __init __maybe_unused
> >>> build_assertions(void)
> >>> * using different PATs will not work.
> >>> */
> >>> BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
> >>> +
> >>> +/*
> >>> + * _PAGE_WB must be zero for several reasons, not least because Linux
> >>> + * assumes it.
> >>> + */
> >>> +BUILD_BUG_ON(_PAGE_WB);
> >>
> >> Strictly speaking this is a requirement only for PV guests. We may not
> >> want to go as far as putting "#ifdef CONFIG_PV" around it, but at least
> >> the code comment (and then also the part of the description referring
> >> to this) will imo want to say so.
> >
> > Does Xen itself depend on this?
>
> With the wording in the description I was going from the assumption that
> you have audited code and found that we properly use _PAGE_* constants
> everywhere.
There could be other hard-coded uses of magic numbers I haven’t found,
and _PAGE_WB is currently zero so I would be quite surpised if no code
in Xen omits it. Linux also has a BUILD_BUG_ON() to check the same
thing.
> >>> +} while (0)
> >>> +
> >>> +/*
> >>> + * If one of these trips, the corresponding _PAGE_* macro is
> >>> inconsistent
> >>> + * with XEN_MSR_PAT. This would cause Xen to use incorrect
> >>> cacheability
> >>> + * flags, with results that are undefined and probably harmful.
> >>
> >> Why "undefined"? They may be wrong / broken, but the result is still well-
> >> defined afaict.
> >
> > “undefined” is meant as “one has violated a core assumption that
> > higher-level stuff depends on, so things can go arbitrarily wrong,
> > including e.g. corrupting memory or data”. Is this accurate? Should I
> > drop the dependent clause, or do you have a suggestion for something
> > better?
>
> s/undefined/unknown/ ?
Will fix in v6.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
Re: [PATCH v5 02/10] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()
On Thu, Dec 22, 2022 at 10:51:08AM +0100, Jan Beulich wrote:
> On 20.12.2022 09:19, Jan Beulich wrote:
> > On 20.12.2022 02:07, Demi Marie Obenour wrote:
> >> get_page_from_l1e() relied on Xen's choice of PAT, which is brittle in
> >> the face of future PAT changes. Instead, compute the actual cacheability
> >> used by the CPU and switch on that, as this will work no matter what PAT
> >> Xen uses.
> >>
> >> No functional change intended. This code is itself questionable and may
> >> be removed in the future, but removing it would be an observable
> >> behavior change and so is out of scope for this patch series.
> >>
> >> Signed-off-by: Demi Marie Obenour
> >> ---
> >> Changes since v4:
> >> - Do not add new pte_flags_to_cacheability() helper, as this code may be
> >> removed in the near future and so adding a new helper for it is a bad
> >> idea.
> >> - Do not BUG() in the event of an unexpected cacheability. This cannot
> >> happen, but it is simpler to force such types to UC than to prove that
> >> the BUG() is not reachable.
> >>
> >> Changes since v3:
> >> - Compute and use the actual cacheability as seen by the processor.
> >>
> >> Changes since v2:
> >> - Improve commit message.
> >> ---
> >> xen/arch/x86/mm.c | 14 --
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> >> index
> >> 78b1972e4170ca9c37c6e64e76e66a7da87f..dba6c77ef2f7ed7fcb7f7e526583ccadd35e62cc
> >> 100644
> >> --- a/xen/arch/x86/mm.c
> >> +++ b/xen/arch/x86/mm.c
> >> @@ -959,14 +959,16 @@ get_page_from_l1e(
> >> flip = _PAGE_RW;
> >> }
> >>
> >> -switch ( l1f & PAGE_CACHE_ATTRS )
> >> +switch ( 0xFF & (XEN_MSR_PAT >> (8 *
> >> pte_flags_to_cacheattr(l1f))) )
> >> {
> >> -case 0: /* WB */
> >> -flip |= _PAGE_PWT | _PAGE_PCD;
> >> +case X86_MT_UC:
> >> +case X86_MT_UCM:
> >> +case X86_MT_WC:
> >> +/* not cacheable */
> >> break;
> >> -case _PAGE_PWT: /* WT */
> >> -case _PAGE_PWT | _PAGE_PAT: /* WP */
> >> -flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
> >> +default:
> >> +/* cacheable */
> >> +flip |= ((l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC);
> >> break;
> >
> > In v4 the comment here was "cacheable, force to UC". The latter aspect is
> > quite relevant (and iirc also what Andrew had asked for to have as a
> > comment). But with this now being the default case, the comment (in either
> > this or the earlier form) would become stale. A forward compatible way of
> > wording this would e.g. be "force any other type to UC". With an adjustment
> > along these lines (which I think could be done while committing)
> > Reviewed-by: Jan Beulich
>
> If you had replied signaling your consent (and perhaps the preferred by you
> wording), I would have committed this. Now it's going to be v6 afaic ...
>
> Jan
Sorry about that. "potentially cacheable, force to UC" is the wording
I have planned for v6, along with "not cacheable, allow" in the other
case. Feel free to go ahead and commit if you want.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default
On Thu, Dec 22, 2022 at 10:48:02AM +0100, Jan Beulich wrote:
> On 20.12.2022 02:07, Demi Marie Obenour wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -145,6 +145,8 @@
> >
> > #ifdef CONFIG_PV
> > #include "pv/mm.h"
> > +bool allow_invalid_cacheability;
>
> static and __ro_after_init please (the former not the least with Misra
> in mind).
Will fix in v6.
> > +boolean_param("allow_invalid_cacheability", allow_invalid_cacheability);
> > #endif
>
> Any new command line option will need to come with an entry in
> docs/misc/xen-command-line.pandoc. Furthermore we're trying to avoid
> underscores in command line option names, when dashes serve the
> purpose quite fine.
Will fix in v6.
> Also please add a blank line at least between #include and your
> addition. Personally I would find it more natural if such a single-use
> control was defined next to the place it is used, i.e.
Will fix in v6.
> > @@ -1343,7 +1345,33 @@ static int promote_l1_table(struct page_info *page)
> > }
> > else
> > {
>
> ... here.
Will move in v6.
> > -switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
> > +l1_pgentry_t l1e = pl1e[i];
> > +
> > +if ( !allow_invalid_cacheability )
> > +{
> > +switch ( l1e.l1 & PAGE_CACHE_ATTRS )
> > +{
> > +case _PAGE_WB:
> > +case _PAGE_UC:
> > +case _PAGE_UCM:
> > +case _PAGE_WC:
> > +case _PAGE_WT:
> > +case _PAGE_WP:
> > +break;
> > +default:
>
> Nit (style): Blank line please between non-fall-through case blocks.
Will fix in v6.
> > +/*
> > + * If we get here, a PV guest tried to use one of the
> > + * reserved values in Xen's PAT. This indicates a bug
> > in
> > + * the guest, so inject #GP to cause the guest to log a
> > + * stack trace.
> > + */
>
> And likely make it die. Which, yes, ...
>
> > +pv_inject_hw_exception(TRAP_gp_fault, 0);
> > +ret = -EINVAL;
>
> ... also may or may not be the result of returning failure here (if the
> guest "checks" for errors by using a BUG()-like construct), but that's
> then more of its own fault than not coping with a non-architectural #GP.
Is there really any architectural behavior here?
> Also wasn't there talk of having this behavior in debug hypervisors
> only? Without that restriction I'm yet less happy with the change ...
My understanding was that Andrew preferred the behavior to be turned on
in release hypervisors too. I am inclined to agree with Andrew, if for
no other reason than that those working on guest OSs do not generally
use debug hypervisors if they are not also working on Xen itself.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On 22.12.2022 10:50, Demi Marie Obenour wrote:
> On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
>> On 20.12.2022 02:07, Demi Marie Obenour wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
>>> return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
>>> }
>>>
>>> +
>>> +/*
>>> + * A bunch of static assertions to check that the XEN_MSR_PAT is valid
>>> + * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
>>> + */
>>> static void __init __maybe_unused build_assertions(void)
>>> {
>>> /*
>>> @@ -6361,6 +6366,72 @@ static void __init __maybe_unused
>>> build_assertions(void)
>>> * using different PATs will not work.
>>> */
>>> BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
>>> +
>>> +/*
>>> + * _PAGE_WB must be zero for several reasons, not least because Linux
>>> + * assumes it.
>>> + */
>>> +BUILD_BUG_ON(_PAGE_WB);
>>
>> Strictly speaking this is a requirement only for PV guests. We may not
>> want to go as far as putting "#ifdef CONFIG_PV" around it, but at least
>> the code comment (and then also the part of the description referring
>> to this) will imo want to say so.
>
> Does Xen itself depend on this?
With the wording in the description I was going from the assumption that
you have audited code and found that we properly use _PAGE_* constants
everywhere.
>>> +} while (0)
>>> +
>>> +/*
>>> + * If one of these trips, the corresponding _PAGE_* macro is
>>> inconsistent
>>> + * with XEN_MSR_PAT. This would cause Xen to use incorrect
>>> cacheability
>>> + * flags, with results that are undefined and probably harmful.
>>
>> Why "undefined"? They may be wrong / broken, but the result is still well-
>> defined afaict.
>
> “undefined” is meant as “one has violated a core assumption that
> higher-level stuff depends on, so things can go arbitrarily wrong,
> including e.g. corrupting memory or data”. Is this accurate? Should I
> drop the dependent clause, or do you have a suggestion for something
> better?
s/undefined/unknown/ ?
Jan
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
Hi Stefano, On 22/12/2022 00:38, Stefano Stabellini wrote: On Tue, 20 Dec 2022, Smith, Jackson wrote: Hi Stefano, On 16/12/2022 01:46, Stefano Stabellini wrote: On Thu, 15 Dec 2022, Julien Grall wrote: On 13/12/2022 19:48, Smith, Jackson wrote: Yes, we are familiar with the "secret-free hypervisor" work. As you point out, both our work and the secret-free hypervisor remove the directmap region to mitigate the risk of leaking sensitive guest secrets. However, our work is slightly different because it additionally prevents attackers from tricking Xen into remapping a guest. I understand your goal, but I don't think this is achieved (see above). You would need an entity to prevent write to TTBR0_EL2 in order to fully protect it. Without a way to stop Xen from reading/writing TTBR0_EL2, we cannot claim that the guest's secrets are 100% safe. But the attacker would have to follow the sequence you outlines above to change Xen's pagetables and remap guest memory before accessing it. It is an additional obstacle for attackers that want to steal other guests' secrets. The size of the code that the attacker would need to inject in Xen would need to be bigger and more complex. Right, that's why I wrote with a bit more work. However, the nuance you mention doesn't seem to be present in the cover letter: "This creates what we call "Software Enclaves", ensuring that an adversary with arbitrary code execution in the hypervisor STILL cannot read/write guest memory." So if the end goal if really to protect against *all* sort of arbitrary code, then I think we should have a rough idea how this will look like in Xen. From a brief look, it doesn't look like it would be possible to prevent modification to TTBR0_EL2 (even from EL3). We would need to investigate if there are other bits in the architecture to help us. Every little helps :-) I can see how making the life of the attacker more difficult is appealing. Yet, the goal needs to be clarified and the risk with the approach acknowledged (see above). You're right, we should have mentioned this weakness in our first email. Sorry about the oversight! This is definitely still a limitation that we have not yet overcome. However, we do think that the increase in attacker workload that you and Stefano are discussing could still be valuable to security conscious Xen users. It would nice to find additional architecture features that we can use to close this hole on arm, but there aren't any that stand out to me either. With this limitation in mind, what are the next steps we should take to support this feature for the xen community? Is this increase in attacker workload meaningful enough to justify the inclusion of VMF in Xen? I think it could be valuable as an additional obstacle for the attacker to overcome. The next step would be to port your series on top of Julien's "Remove the directmap" patch series https://marc.info/?l=xen-devel=167119090721116 Julien, what do you think? If we want Xen to be used in confidential compute, then we need a compelling story and prove that we are at least as secure as other hypervisors. So I think we need to investigate a few areas: * Can we protect the TTBR? I don't think this can be done with the HW. But maybe I overlook it. * Can VMF be extended to more use-cases? For instances, for hypercalls, we could have bounce buffer. * If we can't fully secure VMF, can the attack surface be reduced (e.g. disable hypercalls at runtime/compile time)? Could we use a different architecture (I am thinking something like pKVM [1])? Cheers, [1] https://lwn.net/Articles/836693/ -- Julien Grall
Re: [PATCH v5 02/10] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()
On 20.12.2022 09:19, Jan Beulich wrote:
> On 20.12.2022 02:07, Demi Marie Obenour wrote:
>> get_page_from_l1e() relied on Xen's choice of PAT, which is brittle in
>> the face of future PAT changes. Instead, compute the actual cacheability
>> used by the CPU and switch on that, as this will work no matter what PAT
>> Xen uses.
>>
>> No functional change intended. This code is itself questionable and may
>> be removed in the future, but removing it would be an observable
>> behavior change and so is out of scope for this patch series.
>>
>> Signed-off-by: Demi Marie Obenour
>> ---
>> Changes since v4:
>> - Do not add new pte_flags_to_cacheability() helper, as this code may be
>> removed in the near future and so adding a new helper for it is a bad
>> idea.
>> - Do not BUG() in the event of an unexpected cacheability. This cannot
>> happen, but it is simpler to force such types to UC than to prove that
>> the BUG() is not reachable.
>>
>> Changes since v3:
>> - Compute and use the actual cacheability as seen by the processor.
>>
>> Changes since v2:
>> - Improve commit message.
>> ---
>> xen/arch/x86/mm.c | 14 --
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index
>> 78b1972e4170ca9c37c6e64e76e66a7da87f..dba6c77ef2f7ed7fcb7f7e526583ccadd35e62cc
>> 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -959,14 +959,16 @@ get_page_from_l1e(
>> flip = _PAGE_RW;
>> }
>>
>> -switch ( l1f & PAGE_CACHE_ATTRS )
>> +switch ( 0xFF & (XEN_MSR_PAT >> (8 * pte_flags_to_cacheattr(l1f))) )
>> {
>> -case 0: /* WB */
>> -flip |= _PAGE_PWT | _PAGE_PCD;
>> +case X86_MT_UC:
>> +case X86_MT_UCM:
>> +case X86_MT_WC:
>> +/* not cacheable */
>> break;
>> -case _PAGE_PWT: /* WT */
>> -case _PAGE_PWT | _PAGE_PAT: /* WP */
>> -flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
>> +default:
>> +/* cacheable */
>> +flip |= ((l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC);
>> break;
>
> In v4 the comment here was "cacheable, force to UC". The latter aspect is
> quite relevant (and iirc also what Andrew had asked for to have as a
> comment). But with this now being the default case, the comment (in either
> this or the earlier form) would become stale. A forward compatible way of
> wording this would e.g. be "force any other type to UC". With an adjustment
> along these lines (which I think could be done while committing)
> Reviewed-by: Jan Beulich
If you had replied signaling your consent (and perhaps the preferred by you
wording), I would have committed this. Now it's going to be v6 afaic ...
Jan
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
> On 20.12.2022 02:07, Demi Marie Obenour wrote:
> > It may be desirable to change Xen's PAT for various reasons. This
> > requires changes to several _PAGE_* macros as well. Add static
> > assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
> > macros, and that _PAGE_WB is 0 as required by Linux.
>
> In line with the code comment, perhaps add (not just)"?
Will reword in v6.
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
> > return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
> > }
> >
> > +
> > +/*
> > + * A bunch of static assertions to check that the XEN_MSR_PAT is valid
> > + * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
> > + */
> > static void __init __maybe_unused build_assertions(void)
> > {
> > /*
> > @@ -6361,6 +6366,72 @@ static void __init __maybe_unused
> > build_assertions(void)
> > * using different PATs will not work.
> > */
> > BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
> > +
> > +/*
> > + * _PAGE_WB must be zero for several reasons, not least because Linux
> > + * assumes it.
> > + */
> > +BUILD_BUG_ON(_PAGE_WB);
>
> Strictly speaking this is a requirement only for PV guests. We may not
> want to go as far as putting "#ifdef CONFIG_PV" around it, but at least
> the code comment (and then also the part of the description referring
> to this) will imo want to say so.
Does Xen itself depend on this?
> > +/* A macro to convert from cache attributes to actual cacheability */
> > +#define PAT_ENTRY(v) (0xFF & (XEN_MSR_PAT >> (8 * (v
>
> I don't think the comment is appropriate here. All you do is extract a
> slot from the hard-coded PAT value we use.
Will drop in v6.
> > +/* Validate at compile-time that v is a valid value for a PAT entry */
> > +#define CHECK_PAT_ENTRY_VALUE(v)
> > \
> > +BUILD_BUG_ON((v) < 0 || (v) > 7 ||
> > \
>
> PAT_ENTRY() won't produce negative values, so I don't think "(v) < 0" is
> really needed here. And the "(v) > 7" will imo want replacing by
> "(v) >= X86_NUM_MT".
Will fix in v6.
> > + (v) == X86_MT_RSVD_2 || (v) == X86_MT_RSVD_3)
> > +
> > +/* Validate at compile-time that PAT entry v is valid */
> > +#define CHECK_PAT_ENTRY(v) do {
> > \
> > +BUILD_BUG_ON((v) < 0 || (v) > 7);
> > \
>
> I think this would better be part of PAT_ENTRY(), as the validity of the
> shift there depends on it. If this check is needed in the first place,
> seeing that the macro is #undef-ed right after use below, and hence the
> checks only cover the eight invocations of the macro right here.
>
> > +CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v));
> > \
> > +} while (0);
>
> Nit (style): Missing blanks around 0 and perhaps better nowadays to use
> "false" in such constructs. (Because of other comments this may go away
> here anyway, but there's another similar instance below).
Will fix in v6.
> > +/*
> > + * If one of these trips, the corresponding entry in XEN_MSR_PAT is
> > invalid.
> > + * This would cause Xen to crash (with #GP) at startup.
> > + */
> > +CHECK_PAT_ENTRY(0);
> > +CHECK_PAT_ENTRY(1);
> > +CHECK_PAT_ENTRY(2);
> > +CHECK_PAT_ENTRY(3);
> > +CHECK_PAT_ENTRY(4);
> > +CHECK_PAT_ENTRY(5);
> > +CHECK_PAT_ENTRY(6);
> > +CHECK_PAT_ENTRY(7);
> > +
> > +#undef CHECK_PAT_ENTRY
> > +#undef CHECK_PAT_ENTRY_VALUE
> > +
> > +/* Macro version of page_flags_to_cacheattr(), for use in
> > BUILD_BUG_ON()s */
>
> DYM pte_flags_to_cacheattr()? At which point the macro name wants to
> match and its parameter may also better be named "pte_value"?
Indeed so.
> > +#define PAGE_FLAGS_TO_CACHEATTR(page_value)
> > \
> > +page_value) >> 5) & 4) | (((page_value) >> 3) & 3))
>
> Hmm, yet more uses of magic literal numbers.
I wanted to keep the definition as close to that of
pte_flags_to_cacheattr() as possible.
> > +/* Check that a PAT-related _PAGE_* macro is correct */
> > +#define CHECK_PAGE_VALUE(page_value) do {
> > \
> > +/* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS
> > */\
> > +BUILD_BUG_ON(((_PAGE_##page_value) & PAGE_CACHE_ATTRS) !=
> > \
> > + (_PAGE_##page_value));
> > \
>
> Nit (style): One too many blanks used for indentation.
Will fix in v6.
> > +/* Check that the _PAGE_* are consistent with XEN_MSR_PAT */
> > \
> > +BUILD_BUG_ON(PAT_ENTRY(PAGE_FLAGS_TO_CACHEATTR(_PAGE_##page_value)) !=
> > \
> > +
Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default
On 20.12.2022 02:07, Demi Marie Obenour wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -145,6 +145,8 @@
>
> #ifdef CONFIG_PV
> #include "pv/mm.h"
> +bool allow_invalid_cacheability;
static and __ro_after_init please (the former not the least with Misra
in mind).
> +boolean_param("allow_invalid_cacheability", allow_invalid_cacheability);
> #endif
Any new command line option will need to come with an entry in
docs/misc/xen-command-line.pandoc. Furthermore we're trying to avoid
underscores in command line option names, when dashes serve the
purpose quite fine.
Also please add a blank line at least between #include and your
addition. Personally I would find it more natural if such a single-use
control was defined next to the place it is used, i.e.
> @@ -1343,7 +1345,33 @@ static int promote_l1_table(struct page_info *page)
> }
> else
> {
... here.
> -switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
> +l1_pgentry_t l1e = pl1e[i];
> +
> +if ( !allow_invalid_cacheability )
> +{
> +switch ( l1e.l1 & PAGE_CACHE_ATTRS )
> +{
> +case _PAGE_WB:
> +case _PAGE_UC:
> +case _PAGE_UCM:
> +case _PAGE_WC:
> +case _PAGE_WT:
> +case _PAGE_WP:
> +break;
> +default:
Nit (style): Blank line please between non-fall-through case blocks.
> +/*
> + * If we get here, a PV guest tried to use one of the
> + * reserved values in Xen's PAT. This indicates a bug in
> + * the guest, so inject #GP to cause the guest to log a
> + * stack trace.
> + */
And likely make it die. Which, yes, ...
> +pv_inject_hw_exception(TRAP_gp_fault, 0);
> +ret = -EINVAL;
... also may or may not be the result of returning failure here (if the
guest "checks" for errors by using a BUG()-like construct), but that's
then more of its own fault than not coping with a non-architectural #GP.
Also wasn't there talk of having this behavior in debug hypervisors
only? Without that restriction I'm yet less happy with the change ...
Jan
[xen-4.16-testing test] 175442: trouble: broken/fail/pass
flight 175442 xen-4.16-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/175442/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemut-debianhvm-i386-xsm broken test-amd64-i386-xl-qemut-debianhvm-i386-xsm 5 host-install(5) broken REGR. vs. 175155 test-amd64-i386-xl-xsm broken in 175427 test-amd64-i386-freebsd10-amd64 broken in 175427 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm broken in 175427 test-amd64-i386-xl-qemuu-ovmf-amd64 broken in 175427 test-amd64-i386-xl-vhd broken in 175427 Tests which are failing intermittently (not blocking): test-amd64-i386-xl-vhd 5 host-install(5) broken in 175427 pass in 175442 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 5 host-install(5) broken in 175427 pass in 175442 test-amd64-i386-xl-xsm 5 host-install(5) broken in 175427 pass in 175442 test-amd64-i386-freebsd10-amd64 5 host-install(5) broken in 175427 pass in 175442 test-amd64-i386-xl-qemuu-ovmf-amd64 5 host-install(5) broken in 175427 pass in 175442 test-amd64-i386-livepatch 7 xen-install fail in 175427 pass in 175442 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 175427 test-arm64-arm64-libvirt-raw 12 debian-di-install fail pass in 175427 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 14 migrate-support-check fail in 175427 never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-check fail in 175427 never pass test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175155 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175155 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175155 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175155 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175155 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175155 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175155 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175155 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175155 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175155 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175155 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175155 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On 20.12.2022 02:07, Demi Marie Obenour wrote:
> It may be desirable to change Xen's PAT for various reasons. This
> requires changes to several _PAGE_* macros as well. Add static
> assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
> macros, and that _PAGE_WB is 0 as required by Linux.
In line with the code comment, perhaps add (not just)"?
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
> return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
> }
>
> +
> +/*
> + * A bunch of static assertions to check that the XEN_MSR_PAT is valid
> + * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
> + */
> static void __init __maybe_unused build_assertions(void)
> {
> /*
> @@ -6361,6 +6366,72 @@ static void __init __maybe_unused
> build_assertions(void)
> * using different PATs will not work.
> */
> BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
> +
> +/*
> + * _PAGE_WB must be zero for several reasons, not least because Linux
> + * assumes it.
> + */
> +BUILD_BUG_ON(_PAGE_WB);
Strictly speaking this is a requirement only for PV guests. We may not
want to go as far as putting "#ifdef CONFIG_PV" around it, but at least
the code comment (and then also the part of the description referring
to this) will imo want to say so.
> +/* A macro to convert from cache attributes to actual cacheability */
> +#define PAT_ENTRY(v) (0xFF & (XEN_MSR_PAT >> (8 * (v
I don't think the comment is appropriate here. All you do is extract a
slot from the hard-coded PAT value we use.
> +/* Validate at compile-time that v is a valid value for a PAT entry */
> +#define CHECK_PAT_ENTRY_VALUE(v)
> \
> +BUILD_BUG_ON((v) < 0 || (v) > 7 ||
> \
PAT_ENTRY() won't produce negative values, so I don't think "(v) < 0" is
really needed here. And the "(v) > 7" will imo want replacing by
"(v) >= X86_NUM_MT".
> + (v) == X86_MT_RSVD_2 || (v) == X86_MT_RSVD_3)
> +
> +/* Validate at compile-time that PAT entry v is valid */
> +#define CHECK_PAT_ENTRY(v) do {
> \
> +BUILD_BUG_ON((v) < 0 || (v) > 7);
> \
I think this would better be part of PAT_ENTRY(), as the validity of the
shift there depends on it. If this check is needed in the first place,
seeing that the macro is #undef-ed right after use below, and hence the
checks only cover the eight invocations of the macro right here.
> +CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v));
> \
> +} while (0);
Nit (style): Missing blanks around 0 and perhaps better nowadays to use
"false" in such constructs. (Because of other comments this may go away
here anyway, but there's another similar instance below).
> +/*
> + * If one of these trips, the corresponding entry in XEN_MSR_PAT is
> invalid.
> + * This would cause Xen to crash (with #GP) at startup.
> + */
> +CHECK_PAT_ENTRY(0);
> +CHECK_PAT_ENTRY(1);
> +CHECK_PAT_ENTRY(2);
> +CHECK_PAT_ENTRY(3);
> +CHECK_PAT_ENTRY(4);
> +CHECK_PAT_ENTRY(5);
> +CHECK_PAT_ENTRY(6);
> +CHECK_PAT_ENTRY(7);
> +
> +#undef CHECK_PAT_ENTRY
> +#undef CHECK_PAT_ENTRY_VALUE
> +
> +/* Macro version of page_flags_to_cacheattr(), for use in
> BUILD_BUG_ON()s */
DYM pte_flags_to_cacheattr()? At which point the macro name wants to
match and its parameter may also better be named "pte_value"?
> +#define PAGE_FLAGS_TO_CACHEATTR(page_value)
> \
> +page_value) >> 5) & 4) | (((page_value) >> 3) & 3))
Hmm, yet more uses of magic literal numbers.
> +/* Check that a PAT-related _PAGE_* macro is correct */
> +#define CHECK_PAGE_VALUE(page_value) do {
> \
> +/* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */
> \
> +BUILD_BUG_ON(((_PAGE_##page_value) & PAGE_CACHE_ATTRS) !=
> \
> + (_PAGE_##page_value));
> \
Nit (style): One too many blanks used for indentation.
> +/* Check that the _PAGE_* are consistent with XEN_MSR_PAT */
> \
> +BUILD_BUG_ON(PAT_ENTRY(PAGE_FLAGS_TO_CACHEATTR(_PAGE_##page_value)) !=
> \
> + (X86_MT_##page_value));
> \
Nit (style): Nowadays we tend to consider ## a binary operator just like
e.g. +, and hence it wants to be surrounded by blanks.
> +} while (0)
> +
> +/*
> + * If one of these trips, the corresponding _PAGE_* macro is inconsistent
> + * with XEN_MSR_PAT. This would cause Xen to use incorrect cacheability
> + * flags, with results that are undefined and probably harmful.
Why "undefined"? They may be wrong / broken,
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
Hi Stefano, On 22/12/2022 00:53, Stefano Stabellini wrote: On Tue, 20 Dec 2022, Demi Marie Obenour wrote: On Tue, Dec 20, 2022 at 10:17:24PM +, Smith, Jackson wrote: Hi Stefano, On 16/12/2022 01:46, Stefano Stabellini wrote: On Thu, 15 Dec 2022, Julien Grall wrote: On 13/12/2022 19:48, Smith, Jackson wrote: Yes, we are familiar with the "secret-free hypervisor" work. As you point out, both our work and the secret-free hypervisor remove the directmap region to mitigate the risk of leaking sensitive guest secrets. However, our work is slightly different because it additionally prevents attackers from tricking Xen into remapping a guest. I understand your goal, but I don't think this is achieved (see above). You would need an entity to prevent write to TTBR0_EL2 in order to fully protect it. Without a way to stop Xen from reading/writing TTBR0_EL2, we cannot claim that the guest's secrets are 100% safe. But the attacker would have to follow the sequence you outlines above to change Xen's pagetables and remap guest memory before accessing it. It is an additional obstacle for attackers that want to steal other guests' secrets. The size of the code that the attacker would need to inject in Xen would need to be bigger and more complex. Right, that's why I wrote with a bit more work. However, the nuance you mention doesn't seem to be present in the cover letter: "This creates what we call "Software Enclaves", ensuring that an adversary with arbitrary code execution in the hypervisor STILL cannot read/write guest memory." So if the end goal if really to protect against *all* sort of arbitrary code, then I think we should have a rough idea how this will look like in Xen. From a brief look, it doesn't look like it would be possible to prevent modification to TTBR0_EL2 (even from EL3). We would need to investigate if there are other bits in the architecture to help us. Every little helps :-) I can see how making the life of the attacker more difficult is appealing. Yet, the goal needs to be clarified and the risk with the approach acknowledged (see above). You're right, we should have mentioned this weakness in our first email. Sorry about the oversight! This is definitely still a limitation that we have not yet overcome. However, we do think that the increase in attacker workload that you and Stefano are discussing could still be valuable to security conscious Xen users. It would nice to find additional architecture features that we can use to close this hole on arm, but there aren't any that stand out to me either. With this limitation in mind, what are the next steps we should take to support this feature for the xen community? Is this increase in attacker workload meaningful enough to justify the inclusion of VMF in Xen? Personally, I don’t think so. The kinds of workloads VMF is usable for (no hypercalls) are likely easily portable to other hypervisors, including formally verified microkernels such as seL4 that provide... What other hypervisors might or might not do should not be a factor in this discussion and it would be best to leave it aside. To be honest, Demi has a point. At the moment, VMF is a very niche use-case (see more below). So you would end up to use less than 10% of the normal Xen on Arm code. A lot of people will likely wonder why using Xen in this case? From an AMD/Xilinx point of view, most of our customers using Xen in productions today don't use any hypercalls in one or more of their VMs. This suggests a mix of guests are running (some using hypercalls and other not). It would not be possible if you were using VMF. Xen is great for these use-cases and it is rather common in embedded. It is certainly a different configuration from what most are come to expect from Xen on the server/desktop x86 side. There is no question that guests without hypercalls are important for Xen on ARM. > As a Xen community we have a long history and strong interest in making Xen more secure and also, more recently, safer (in the ISO 26262 safety-certification sense). The VMF work is very well aligned with both of these efforts and any additional burder to attackers is certainly good for Xen. I agree that we have a strong focus on making Xen more secure. However, we also need to look at the use cases for it. As it stands, there will no: - IOREQ use (don't think about emulating TPM) - GICv3 ITS - stage-1 SMMUv3 - decoding of instructions when there is no syndrome - hypercalls (including event channels) - dom0 That's a lot of Xen features that can't be used. Effectively you will make Xen more "secure" for a very few users. Now the question is what changes are necessary and how to make them to the codebase. And if it turns out that some of the changes are not applicable or too complex to accept, the decision will be made purely from a code maintenance point of view and will have nothing to do with VMs making no hypercalls being
[ovmf test] 175452: all pass - PUSHED
flight 175452 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175452/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf ec87305f90d90096aac2a4d91e3f6556e2ecd6b9 baseline version: ovmf 129404f6e4395008ac0045e7e627edbba2a1e064 Last test of basis 175448 2022-12-22 01:42:28 Z0 days Testing same since 175452 2022-12-22 07:10:57 Z0 days1 attempts People who touched revisions under test: KasimX Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 129404f6e4..ec87305f90 ec87305f90d90096aac2a4d91e3f6556e2ecd6b9 -> xen-tested-master
Re: [RFC PATCH 01/18] arm: cppcheck: misra rule 20.7 deviations for alternative.h
On 22.12.2022 03:01, Stefano Stabellini wrote: > What do you guys think? Nice automatic 20.7 scans for all patches and > for staging, but with the undesirable false-positive comments, or > no-20.7 scans but no comments in the tree? Anything automatic which then also bugs submitters about issues should have as few false positives as possible. Otherwise people may quickly get into the habit of ignoring such reports. Jan
Re: [PATCH 0/6] enforce use of G_GNUC_PRINTF annotations
Queued, thanks. Paolo
Re: [Intel-gfx] [cache coherency bug] i915 and PAT attributes
On Fri, Dec 16, 2022 at 03:30:13PM +, Andrew Cooper wrote:
> On 08/12/2022 1:55 pm, Marek Marczykowski-Górecki wrote:
> > Hi,
> >
> > There is an issue with i915 on Xen PV (dom0). The end result is a lot of
> > glitches, like here: https://openqa.qubes-os.org/tests/54748#step/startup/8
> > (this one is on ADL, Linux 6.1-rc7 as a Xen PV dom0). It's using Xorg
> > with "modesetting" driver.
> >
> > After some iterations of debugging, we narrowed it down to i915 handling
> > caching. The main difference is that PAT is setup differently on Xen PV
> > than on native Linux. Normally, Linux does have appropriate abstraction
> > for that, but apparently something related to i915 doesn't play well
> > with it. The specific difference is:
> > native linux:
> > x86/PAT: Configuration [0-7]: WB WC UC- UC WB WP UC- WT
> > xen pv:
> > x86/PAT: Configuration [0-7]: WB WT UC- UC WC WP UC UC
> > ~~ ~~ ~~ ~~
> >
> > The specific impact depends on kernel version and the hardware. The most
> > severe issues I see on >=ADL, but some older hardware is affected too -
> > sometimes only if composition is disabled in the window manager.
> > Some more information is collected at
> > https://github.com/QubesOS/qubes-issues/issues/4782 (and few linked
> > duplicates...).
> >
> > Kind-of related commit is here:
> > https://github.com/torvalds/linux/commit/bdd8b6c98239cad ("drm/i915:
> > replace X86_FEATURE_PAT with pat_enabled()") - it is the place where
> > i915 explicitly checks for PAT support, so I'm cc-ing people mentioned
> > there too.
> >
> > Any ideas?
> >
> > The issue can be easily reproduced without Xen too, by adjusting PAT in
> > Linux:
> > -8<-
> > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> > index 66a209f7eb86..319ab60c8d8c 100644
> > --- a/arch/x86/mm/pat/memtype.c
> > +++ b/arch/x86/mm/pat/memtype.c
> > @@ -400,8 +400,8 @@ void pat_init(void)
> > * The reserved slots are unused, but mapped to their
> > * corresponding types in the presence of PAT errata.
> > */
> > - pat = PAT(0, WB) | PAT(1, WC) | PAT(2, UC_MINUS) | PAT(3, UC) |
> > - PAT(4, WB) | PAT(5, WP) | PAT(6, UC_MINUS) | PAT(7, WT);
> > + pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
> > + PAT(4, WC) | PAT(5, WP) | PAT(6, UC) | PAT(7, UC);
> > }
> >
> > if (!pat_bp_initialized) {
> > -8<-
> >
>
> Hello, can anyone help please?
>
> Intel's CI has taken this reproducer of the bug, and confirmed the
> regression.
> https://lore.kernel.org/intel-gfx/Y5Hst0bCxQDTN7lK@mail-itl/T/#m4480c15a0d117dce6210562eb542875e757647fb
>
> We're reasonably confident that it is an i915 bug (given the repro with
> no Xen in the mix), but we're out of any further ideas.
I don't think we have any code that assumes anything about the PAT,
apart from WC being available (which seems like it should still be
the case with your modified PAT). I suppose you'll just have to
start digging from pgprot_writecombine()/noncached() and make sure
everything ends up using the correct PAT entry.
--
Ville Syrjälä
Intel
Re: [PATCH 6/8] x86/shadow: adjust and move sh_type_to_size[]
On 21.12.2022 19:58, Andrew Cooper wrote:
> On 21/12/2022 1:27 pm, Jan Beulich wrote:
>> Drop the SH_type_none entry - there are no allocation attempts with
>> this type, and there also shouldn't be any. Adjust the shadow_size()
>> alternative path to match that change. Also generalize two related
>> assertions.
>>
>> While there move the entire table and the respective part of the comment
>> there to hvm.c, resulting in one less #ifdef. In the course of the
>> movement switch to using designated initializers.
>>
>> Signed-off-by: Jan Beulich
>
> Reviewed-by: Andrew Cooper , although ...
Thanks.
>> --- a/xen/arch/x86/mm/shadow/hvm.c
>> +++ b/xen/arch/x86/mm/shadow/hvm.c
>> @@ -33,6 +33,37 @@
>>
>> #include "private.h"
>>
>> +/*
>> + * This table shows the allocation behaviour of the different modes:
>> + *
>> + * Xen paging 64b 64b 64b
>> + * Guest paging32b pae 64b
>> + * PV or HVM HVM HVM *
>> + * Shadow paging pae pae 64b
>> + *
>> + * sl1 size 8k 4k 4k
>> + * sl2 size16k 4k 4k
>> + * sl3 size --4k
>> + * sl4 size --4k
>> + */
>> +const uint8_t sh_type_to_size[] = {
>> +[SH_type_l1_32_shadow] = 2,
>> +[SH_type_fl1_32_shadow] = 2,
>> +[SH_type_l2_32_shadow] = 4,
>> +[SH_type_l1_pae_shadow] = 1,
>> +[SH_type_fl1_pae_shadow] = 1,
>> +[SH_type_l2_pae_shadow] = 1,
>> +[SH_type_l1_64_shadow] = 1,
>> +[SH_type_fl1_64_shadow] = 1,
>> +[SH_type_l2_64_shadow] = 1,
>> +[SH_type_l2h_64_shadow] = 1,
>> +[SH_type_l3_64_shadow] = 1,
>> +[SH_type_l4_64_shadow] = 1,
>> +[SH_type_p2m_table] = 1,
>> +[SH_type_monitor_table] = 1,
>> +[SH_type_oos_snapshot] = 1,
>
> ... this feels like it's wanting to turn into a (1 + ...) expression. I
> can't see anything that prevents us from reordering the SH_type
> constants, but
>
> 1 + (idx == 1 /* l1 */ || idx == /* fl1 */) + 2 * (idx == 3 /* l2 */);
>
> doesn't obviously simplify further.
But that would then undermine the cases where the function returns 0,
which the two (generalized in this change) assertions actually check
for not having got back. This would also become relevant for the l2h
slot, which - if not right here (see the respective remark) - will
become zero in a subsequent patch when !PV32.
Jan
Re: [PATCH 3/8] x86/paging: move update_paging_modes() hook
On 21.12.2022 18:43, Andrew Cooper wrote: > On 21/12/2022 1:25 pm, Jan Beulich wrote: >> The hook isn't mode dependent, hence it's misplaced in struct >> paging_mode. (Or alternatively I see no reason why the alloc_page() and >> free_page() hooks don't also live there.) Move it to struct >> paging_domain. >> >> While there rename the hook and HAP's as well as shadow's hook functions >> to use singular; I never understood why plural was used. (Renaming in >> particular the wrapper would be touching quite a lot of other code.) > > There are always two modes; Xen's, and the guest's. > > This was probably more visible back in the 32-bit days, but remnants of > it are still around with the fact that struct vcpu needs to be below the > 4G boundary for the PDPTRs for when the guest isn't in Long Mode. > > HAP also hides it fairly well given the uniformity of EPT/NPT (and > always 4 levels in a 64-bit Xen), but I suspect it will become more > visible again when we start supporting LA57. So does this boil down to a request to undo the rename? Or undo it just for shadow code (as the HAP function really does only one thing)? As to LA57, I'm not convinced it'll become more visible again then, but of course without actually doing that work it's all hand-waving anyway. >> --- a/xen/arch/x86/mm/shadow/none.c >> +++ b/xen/arch/x86/mm/shadow/none.c >> @@ -27,6 +32,9 @@ int shadow_domain_init(struct domain *d) >> }; >> >> paging_log_dirty_init(d, _none_ops); >> + >> +d->arch.paging.update_paging_mode = _update_paging_mode; >> + >> return is_hvm_domain(d) ? -EOPNOTSUPP : 0; > > I know you haven't changed the logic here, but this hook looks broken. > Why do we fail it right at the end for HVM domains? It's been a long time, but I guess my thinking back then was that it's better to put in place pointers which other code may rely on being non- NULL. Jan
