Re: [PATCH v2 01/11] kexec: introduce kexec_ops struct
On 22/11/12 17:47, H. Peter Anvin wrote: > The other thing that should be considered here is how utterly > preposterous the notion of doing in-guest crash dumping is in a system > that contains a hypervisor. The reason for kdump is that on bare metal > there are no other options, but in a hypervisor system the right thing > should be for the hypervisor to do the dump (possibly spawning a clean > I/O domain if the I/O domain is necessary to access the media.) > > There is absolutely no reason to have a crashkernel sitting around in > each guest, consuming memory, and possibly get corrupt. > > -hpa > I agree that regular guests should not be using the kexec/kdump. However, this patch series is required for allowing a pvops kernel to be a crash kernel for Xen, which is very important from dom0/Xen's point of view. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 01/11] kexec: introduce kexec_ops struct
On 22/11/2012 17:47, H. Peter Anvin wrote: > The other thing that should be considered here is how utterly > preposterous the notion of doing in-guest crash dumping is in a system > that contains a hypervisor. The reason for kdump is that on bare metal > there are no other options, but in a hypervisor system the right thing > should be for the hypervisor to do the dump (possibly spawning a clean > I/O domain if the I/O domain is necessary to access the media.) > > There is absolutely no reason to have a crashkernel sitting around in > each guest, consuming memory, and possibly get corrupt. > > -hpa > (Your reply to my email which I can see on the xen devel archive appears to have gotten lost somewhere inside the citrix email system, so apologies for replying out of order) The kdump kernel loaded by dom0 is for when Xen crashes, not for when dom0 crashes (although a dom0 crash does admittedly lead to a Xen crash) There is no possible way it could be a separate domain; Xen completely ceases to function as soon as jumps to the entry point of the kdump image. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 01/11] kexec: introduce kexec_ops struct
On 23/11/2012 01:38, H. Peter Anvin wrote: > I still don't really get why it can't be isolated from dom0, which would make > more sense to me, even for a Xen crash. > The crash region (as specified by crashkernel= on the Xen command line) is isolated from dom0. dom0 (using the kexec utility etc) has the task of locating the Xen crash notes (using the kexec hypercall interface), constructing a binary blob containing kernel, initram and gubbins, and asking Xen to put this blob in the crash region (again, using the kexec hypercall interface). I do not see how this is very much different from the native case currently (although please correct me if I am misinformed). Linux has extra work to do by populating /proc/iomem with the Xen crash regions boot (so the kexec utility can reference their physical addresses when constructing the blob), and should just act as a conduit between the kexec system call and the kexec hypercall to load the blob. For within-guest kexec/kdump functionality, I agree that it is barking mad. However, we do see cloud operators interested in the idea so VM administrators can look after their crashes themselves. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 00/11] xen: Initial kexec/kdump implementation
On 27/12/2012 07:53, Eric W. Biederman wrote: > The syscall ABI still has the wrong semantics. > > Aka totally unmaintainable and umergeable. > > The concept of domU support is also strange. What does domU support even > mean, when the dom0 support is loading a kernel to pick up Xen when Xen falls > over. There are two requirements pulling at this patch series, but I agree that we need to clarify them. When dom0 loads a crash kernel, it is loading one for Xen to use. As a dom0 crash causes a Xen crash, having dom0 set up a kdump kernel for itself is completely useless. This ability is present in "classic Xen dom0" kernels, but the feature is currently missing in PVOPS. Many cloud customers and service providers want the ability for a VM administrator to be able to load a kdump/kexec kernel within a domain[1]. This allows the VM administrator to take more proactive steps to isolate the cause of a crash, the state of which is most likely discarded while tearing down the domain. The result being that as far as Xen is concerned, the domain is still alive, while the kdump kernel/environment can work its usual magic. I am not aware of any feature like this existing in the past. ~Andrew [1] http://lists.xen.org/archives/html/xen-devel/2012-11/msg01274.html > > I expect a lot of decisions about what code can be shared and what code can't > is going to be driven by the simple question what does the syscall mean. > > Sharing machine_kexec.c and relocate_kernel.S does not make much sense to me > when what you are doing is effectively passing your arguments through to the > Xen version of kexec. > > Either Xen has it's own version of those routines or I expect the Xen version > of kexec is buggy. I can't imagine what sharing that code would mean. By > the same token I can't any need to duplicate the code either. > > Furthermore since this is just passing data from one version of the syscall > to another I expect you can share the majority of the code across all > architectures that implement Xen. The only part I can see being arch > specific is the Xen syscall stub. > > With respect to the proposed semantics of silently giving the kexec system > call different meaning when running under Xen, > /sbin/kexec has to act somewhat differently when loading code into the Xen > hypervisor so there is no point not making that explicit in the ABI. > > Eric > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH v3 00/11] xen: Initial kexec/kdump implementation
On 27/12/12 18:02, Eric W. Biederman wrote: Andrew Cooper writes: On 27/12/2012 07:53, Eric W. Biederman wrote: The syscall ABI still has the wrong semantics. Aka totally unmaintainable and umergeable. The concept of domU support is also strange. What does domU support even mean, when the dom0 support is loading a kernel to pick up Xen when Xen falls over. There are two requirements pulling at this patch series, but I agree that we need to clarify them. It probably make sense to split them apart a little even. Thinking about this split, there might be a way to simply it even more. /sbin/kexec can load the "Xen" crash kernel itself by issuing hypercalls using /dev/xen/privcmd. This would remove the need for the dom0 kernel to distinguish between loading a crash kernel for itself and loading a kernel for Xen. Or is this just a silly idea complicating the matter? ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH v3 00/11] xen: Initial kexec/kdump implementation
On 07/01/13 10:25, Ian Campbell wrote: On Fri, 2013-01-04 at 19:11 +, Konrad Rzeszutek Wilk wrote: On Fri, Jan 04, 2013 at 06:07:51PM +0100, Daniel Kiper wrote: Because current KEXEC_CMD_kexec_load does not load kernel image and other things into Xen memory. It means that it should live somewhere in dom0 Linux kernel memory. We could have a very simple hypercall which would have: struct fancy_new_hypercall { xen_pfn_t payload; // IN This would have to be XEN_GUEST_HANDLE(something) since userspace cannot figure out what pfns back its memory. In any case since the hypervisor is going to want to copy the data into the crashkernel space a virtual address is convenient to have. ssize_t len; // IN #define DATA (1<<1) #define DATA_EOF (1<<2) #define DATA_KERNEL (1<<3) #define DATA_RAMDISK (1<<4) unsigned int flags; // IN unsigned int status; // OUT }; which would in a loop just iterate over the payloads and let the hypervisor stick it in the crashkernel space. This is all hand-waving of course. There probably would be a need to figure out how much space you have in the reserved Xen's 'crashkernel' memory region too. This is probably a mad idea but it's Monday morning and I'm sleep deprived so I'll throw it out there... What about adding DOMID_KEXEC (similar DOMID_IO etc)? This would allow dom0 to map the kexec memory space with the usual privcmd mmap hypercalls and build things in it directly. OK, I suspect this might not be practical for a variety of reasons (lack of a p2m for such domains so no way to find out the list of mfns, dom0 userspace simply doesn't have sufficient context to write sensible things here, etc) but maybe someone has a better head on today... Ian. Given that /sbin/kexec creates a binary blob in memory, surely the most simple thing is to get it to suitably mlock() the region and give a list of VAs to the hypervisor. This way, Xen can properly take care of what it does with information and where. For example, at the moment, allowing dom0 to choose where gets overwritten in the Xen crash area is a recipe for disaster if a crash occurs midway through loading/reloading the crash kernel. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [GIT PULL] x86/mm changes for v3.9-rc1
On 22/02/13 17:53, Yinghai Lu wrote: > On Fri, Feb 22, 2013 at 9:24 AM, Konrad Rzeszutek Wilk > wrote: >> On Fri, Feb 22, 2013 at 11:55:31AM -0500, Konrad Rzeszutek Wilk wrote: >>> On Thu, Feb 21, 2013 at 04:34:06PM -0800, H. Peter Anvin wrote: Hi Linus, This is a huge set of several partly interrelated (and concurrently developed) changes, which is why the branch history is messier than one would like. The *really* big items are two humonguous patchsets mostly developed by Yinghai Lu at my request, which completely revamps the way we create initial page tables. In particular, rather than estimating how much memory we will need for page tables and then build them into that memory -- a calculation that has shown to be incredibly fragile -- we now build them (on 64 bits) with the aid of a "pseudo-linear mode" -- a #PF handler which creates temporary page tables on demand. This has several advantages: 1. It makes it much easier to support things that need access to data very early (a followon patchset uses this to load microcode way early in the kernel startup). 2. It allows the kernel and all the kernel data objects to be invoked from above the 4 GB limit. This allows kdump to work on very large systems. 3. It greatly reduces the difference between Xen and native (Xen's equivalent of the #PF handler are the temporary page tables created by the domain builder), eliminating a bunch of fragile hooks. The patch series also gets us a bit closer to W^X. Additional work in this pull is the 64-bit get_user() work which you were also involved with, and a bunch of cleanups/speedups to __phys_addr()/__pa(). >>> Looking at figuring out which of the patches in the branch did this, but >>> with this merge I am getting a crash with a very simple PV guest (booted >>> with >>> one 1G): >>> >>> Call Trace: >>> [] xen_get_user_pgd+0x5a <-- >>> [] xen_get_user_pgd+0x5a >>> [] xen_write_cr3+0x77 >>> [] init_mem_mapping+0x1f9 >>> [] setup_arch+0x742 >>> [] printk+0x48 >>> [] start_kernel+0x90 >>> [] __add_preferred_console.clone.1+0x9b >>> [] x86_64_start_reservations+0x2a >>> [] xen_start_kernel+0x564 >>> >>> And the hypervisor says: >>> (XEN) d7:v0: unhandled page fault (ec=) >>> (XEN) Pagetable walk from ea05b2d0: >>> (XEN) L4[0x1d4] = >>> (XEN) domain_crash_sync called from entry.S >>> (XEN) Domain 7 (vcpu#0) crashed on cpu#3: >>> (XEN) [ Xen-4.2.0 x86_64 debug=n Not tainted ] >>> (XEN) CPU:3 >>> (XEN) RIP:e033:[] >>> (XEN) RFLAGS: 0206 EM: 1 CONTEXT: pv guest >>> (XEN) rax: ea00 rbx: 01a0c000 rcx: 8000 >>> (XEN) rdx: 0005b2a0 rsi: 01a0c000 rdi: >>> (XEN) rbp: 81a01dd8 rsp: 81a01d90 r8: >>> (XEN) r9: 1001 r10: r11: >>> (XEN) r12: r13: 0010 r14: >>> (XEN) r15: 0010 cr0: 8005003b cr4: 000406f0 >>> (XEN) cr3: 000411165000 cr2: ea05b2d0 >>> (XEN) ds: es: fs: gs: ss: e02b cs: e033 >>> (XEN) Guest stack trace from rsp=81a01d90: >>> (XEN)8000 8103feba >>> (XEN)0001e030 00010006 81a01dd8 e02b >> Here is a better serial log of the crash (just booting a normal Xen 4.1 + >> initial >> kernel with 8GB): >> >> PXELINUX 3.82 2009-06-09 Copyright (C) 1994-2009 H. Peter Anvin et al >> boot: >> Loading xen.gz... ok >> Loading vmlinuz... ok >> Loading initramfs.cpio.gz... ok >> __ ___ __ >> \ \/ /___ _ __ | || | / | | ___|_ __ _ __ ___ >> \ // _ \ '_(_)_(_)/ | .__/|_| \___| >>|_| >> (XEN) Xen version 4.1.5-pre (kon...@dumpdata.com) (gcc version 4.4.4 >> 20100503 (Red Hat 4.4.4-2) (GCC) ) Fri Feb 22 11:37:00 EST 2013 >> (XEN) Latest ChangeSet: Fri Feb 15 15:31:55 2013 +0100 23459:9f12bdd6b7f0 >> (XEN) Console output is synchronous. >> (XEN) Bootloader: unknown >> (XEN) Command line: cpuinfo conring_size=1048576 sync_console >> cpufreq=verbose com1=115200,8n1 console=com1,vga loglvl=all guest_loglvl=all >> (XEN) Video information: >> (XEN) VGA is text mode 80x25, font 8x16 >> (XEN) VBE/DDC methods: none; EDID transfer time: 0 seconds >> (XEN) EDID info not retrieved because no DDC retrieval method detected >> (XEN) Disc information: >> (XEN) Found 1 MBR signatures >> (XEN) Found 1 EDD information structures >> (XEN) Xen-e820 RAM map: >> (XEN) - 0009ec00 (usable) >> (XEN) 0009ec00 - 000a (reserved) >> (XEN) 000e - 00
Re: [Xen-devel] [RFC] Hypervisor RNG and enumeration
On 29/10/14 05:19, Andy Lutomirski wrote: > Here's a draft CommonHV spec. It's also on github: > > https://github.com/amluto/CommonHV > > So far, this provides a two-way RNG interface, a way to detect it, and > a way to detect other hypervisor leaves. The latter is because, after > both the enormous public thread and some private discussions, it seems > that detection of existing CPUID paravirt leaves is annoying and > inefficient. If we're going to define some cross-vendor CPUID leaves, > it seems like it would be useful to offer a way to quickly enumerate > other leaves. > > I've been told the AMD intends to update their manual to match Intel's > so that hypervisors can use the entire 0x4F?? CPUID range. I have > intentionally not fixed an MSR value for the RNG because the range of > allowed MSRs is very small in both the Intel and AMD manuals. If any > given hypervisor wants to ignore that small range and advertise a > higher-numbered MSR, it is welcome to, but I don't want to codify > something that doesn't comply with the manuals. > > Here's the draft. Comments? To the people who work on various > hypervisors: Would you implement this? Do you like it? Is there > anything, major or minor, that you'd like to see changed? Do you > think that this is a good idea at all? As a first pass, it looks like a plausible idea. I do however have come comments. > > I've tried to get good coverage of various hypervisors. There are > Hyper-V, VMWare, KVM, and Xen people on the cc list. > > Thanks, > Andy > > > > CommonHV, a common hypervisor interface > === > > This is CommonHV draft 1. > > The CommonHV specification is Copyright (c) 2014 Andrew Lutomirski. > > Licensing will be determined soon. The license is expected to be extremely > liberal. I am currently leaning towards CC-BY-SA for the specification and > an explicit license permitting anyone to implement the specification > with no restrictions whatsoever. > > I have not patented, nor do I intend to patent, anything required to implement > this specification. I am not aware of any current or future intellectual > property rights that would prevent a royalty-free implementation of > this specification. > > I would like to find a stable, neutral steward of this specification > going forward. Help with this would be much appreciated. > > Scope > - > > CommonHV is a simple interface for communication > between hypervisors and their guests. > > CommonHV is intended to be very simple and to avoid interfering with > existing paravirtual interfaces. To that end, its scope is limited. > CommonHV does only two types of things: > > * It provides a way to enumerate other paravirtual interfaces. > * It provides a small, extensible set of paravirtual features that do not > modify or replace standard system functionality. > > For example, CommonHV does not and will not define anything related to > interrupt handling or virtual CPU management. > > For now, CommonHV is only applicable to the x86 platform. > > Discovery > - > > A CommonHV hypervisor MUST set the hypervisor bit (bit 31 in CPUID.1H.0H.ECX) > and provide the CPUID leaf 4F00H, containing: > > * CPUID.4F00H.0H.EAX = max_commonhv_leaf > * CPUID.4F00H.0H.EBX = 0x6D6D6F43 > * CPUID.4F00H.0H.ECX = 0x56486E6F > * CPUID.4F00H.0H.EDX = 0x66746e49 > > EBX, ECX, and EDX form the string "CommonHVIntf" in little-endian ASCII. While testing various nested combinations, XenServer has found that modern Windows Server versions must have the hypervisor bit hidden from them for them to be happy running HyperV, despite the fact that they will make use of the Viridian virtual extensions also provided. As a result, while it is certainly advisable for the hypervisor bit to be set, CommonHV should be available to be found by paravirtualised drivers inside an OS which can't cope with the hypervisor bit set. > > max_commonhv_leaf MUST be a number between 0x4F00 and 0x4FFF. It > indicates the largest leaf defined in this specification that is provided. > Any leaves described in this specification with EAX values that exceed > max_commonhv_leaf MUST be handled by guests as though they contain > all zeros. > > CPUID leaf 4F01H: hypervisor interface enumeration > -- > > If max_commonhv_leaf >= 0x4F01, CommonHV provides a list of tuples > (location, signature). Each tuple indicates the presence of another > paravirtual interface identified by the signature at the indicated > CPUID location. It is expected that CPUID.location.0H will have > (EBX, ECX, EDX) == signature, although whether this is required > is left to the specification associated with the given signature. > > If the list contains N tuples, then, for each 0 <= i < N: > > * CPUID.4F01H.i.EBX, CPUID.4F01H.i.ECX, and CPUID.4F01H.i.EDX > are the signature. > * CPUID.4F01H.i.EAX is the
Re: [Xen-devel] [PATCH v2 0/3] Fix and cleanup for 32-bit PV sysexit
On 19/11/15 22:07, Andy Lutomirski wrote: > On Thu, Nov 19, 2015 at 1:55 PM, Boris Ostrovsky > wrote: >> The first patch fixes Xen PV regression introduced by 32-bit rewrite. Unlike >> the >> earlier version it uses ALTERNATIVE instruction and avoids using xen_sysexit >> (and sysret32 in compat mode) pv ops, as suggested by Andy. >> >> As result of this patch irq_enable_sysexit and usergs_sysret32 pv ops are not >> used anymore by anyone and so can be removed. > This whole series is: > > Acked-by: Andy Lutomirski > > Now I just have to sucker someone into getting rid of > PARAVIRT_ADJUST_EXCEPTION_FRAME (by using stub entries) and the > overcomplicated syscall entry stuff. :) Looking at this, it should be quite easy now. ALTERNATIVE "", "pop %rcx; %pop %11", X86_FEATURE_XENPV (Completely untested) > And whoever gets rid of > PARAVIRT_ADJUST_EXCEPTION_FRAME gets to wonder why it doesn't crash > and burn for NMIs on Xen, since I'm reasonably confident that it can't > possibly be correct. The Xen PV ABI only has a single kernel stack pointer which may be registered. There is no equivalent of an IST, so if a second fault occurs, it is delivered normally on the current stack. By the looks of it, the other NMI handling is ambivalent to the fact that it isn't really on an IST stack under Xen. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
On 20/12/15 09:25, Michael S. Tsirkin wrote: > On Thu, Dec 17, 2015 at 03:39:10PM +0100, Peter Zijlstra wrote: >> On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote: >>> On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote: You could of course go fix that instead of mutilating things into sort-of functional state. >>> Yes, we'd just need to touch all architectures, all for >>> the sake of UP which almost no one uses. >>> Basically, we need APIs that explicitly are >>> for talking to another kernel on a different CPU on >>> the same SMP system, and implemented identically >>> between CONFIG_SMP and !CONFIG_SMP on all architectures. >>> >>> Do you think this is something of general usefulness, >>> outside virtio? >> I'm not aware of any other case, but if there are more parts of virt >> that need this then I see no problem adding it. > When I wrote this, I assumed there are no other users, and I'm still not > sure there are other users at the moment. Do you see a problem then? > >> That is, virt in general is the only use-case that I can think of, >> because this really is an artifact of interfacing with an SMP host while >> running an UP kernel. > Or another guest kernel on an SMP host. > >> But I'm really not familiar with virt, so I do not know if there's more >> sites outside of virtio that could use this. >> Touching all archs is a tad tedious, but its fairly straight forward. > So I looked and I was only able to find other another possible user in Xen. > > Cc Xen folks. > > I noticed that drivers/xen/xenbus/xenbus_comms.c uses > full memory barriers to communicate with the other side. > For example: > > /* Must write data /after/ reading the consumer index. * */ > mb(); > > memcpy(dst, data, avail); > data += avail; > len -= avail; > > /* Other side must not see new producer until data is * > there. */ > wmb(); > intf->req_prod += avail; > > /* Implies mb(): other side will see the updated producer. */ > notify_remote_via_evtchn(xen_store_evtchn); > > To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb > would be sufficient, so mb() and wmb() here are only needed if > a non-SMP guest runs on an SMP host. > > Is my analysis correct? Correct. The reason full barriers are used is so non-SMP guests still function correctly. It is certainly inefficient. > > So what I'm suggesting is something like the below patch, > except instead of using virtio directly, a new set of barriers > that behaves identically for SMP and non-SMP guests will be introduced. > > And of course the weak barriers flag is not needed for Xen - > that's a virtio only thing. > > For example: > > smp_pv_wmb() > smp_pv_rmb() > smp_pv_mb() > > I'd like to get confirmation from Xen folks before posting > this patchset. > > Comments/suggestions? Very much +1 for fixing this. Those names would be fine, but they do add yet another set of options in an already-complicated area. An alternative might be to have the regular smp_{w,r,}mb() not revert back to nops if CONFIG_PARAVIRT, or perhaps if pvops have detected a non-native environment. (I don't know how feasible this suggestion is, however.) ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH v2] x86/paravirt: Don't make vcpu_is_preempted() a callee-save function
On 14/02/17 14:46, Waiman Long wrote: > On 02/14/2017 04:39 AM, Peter Zijlstra wrote: >> On Mon, Feb 13, 2017 at 05:34:01PM -0500, Waiman Long wrote: >>> It is the address of &steal_time that will exceed the 32-bit limit. >> That seems extremely unlikely. That would mean we have more than 4G >> worth of per-cpu variables declared in the kernel. > I have some doubt about if the compiler is able to properly use > RIP-relative addressing for this. Anyway, it seems like constraints > aren't allowed for asm() when not in the function context, at least for > the the compiler that I am using (4.8.5). So it is a moot point. You can work the issue of not having parameters in a plain asm() statement by using an asm-offset, stringizing it, and have C put the string fragments back together. "cmpb $0, " STR(STEAL_TIME_preempted) "(%rax);" ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
On 12/10/17 20:11, Boris Ostrovsky wrote: > On 10/06/2017 10:32 AM, Josh Poimboeuf wrote: >> On Thu, Oct 05, 2017 at 04:35:03PM -0400, Boris Ostrovsky wrote: #ifdef CONFIG_PARAVIRT +/* + * Paravirt alternatives are applied much earlier than normal alternatives. + * They are only applied when running on a hypervisor. They replace some + * native instructions with calls to pv ops. + */ +void __init apply_pv_alternatives(void) +{ + setup_force_cpu_cap(X86_FEATURE_PV_OPS); >>> Not for Xen HVM guests. >> From what I can tell, HVM guests still use pv_time_ops and >> pv_mmu_ops.exit_mmap, right? >> + apply_alternatives(__pv_alt_instructions, __pv_alt_instructions_end); +} >>> This is a problem (at least for Xen PV guests): >>> apply_alternatives()->text_poke_early()->local_irq_save()->...'cli'->death. >> Ah, right. >> >>> It might be possible not to turn off/on the interrupts in this >>> particular case since the guest probably won't be able to handle an >>> interrupt at this point anyway. >> Yeah, that should work. For Xen and for the other hypervisors, this is >> called well before irq init, so interrupts can't be handled yet anyway. > There is also another problem: > > [1.312425] general protection fault: [#1] SMP > [1.312901] Modules linked in: > [1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6 > [1.313878] task: 88003e2c task.stack: c938c000 > [1.314360] RIP: 1e030:entry_SYSCALL_64_fastpath+0x1/0xa5 > [1.314854] RSP: e02b:c938ff50 EFLAGS: 00010046 > [1.315336] RAX: 000c RBX: 55f550168040 RCX: > 7fcfc959f59a > [1.315827] RDX: RSI: RDI: > > [1.316315] RBP: 000a R08: 037f R09: > 0064 > [1.316805] R10: 1f89cbf5 R11: 88003e2c R12: > 7fcfc958ad60 > [1.317300] R13: R14: 55f550185954 R15: > 1000 > [1.317801] FS: () GS:88003f80() > knlGS: > [1.318267] CS: e033 DS: ES: CR0: 80050033 > [1.318750] CR2: 7fcfc97ab218 CR3: 3c88e000 CR4: > 00042660 > [1.319235] Call Trace: > [1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48 > 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00 > 00 50 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5 > [1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: c938ff50 > [1.344255] ---[ end trace d7cb8cd6cd7c294c ]--- > [1.345009] Kernel panic - not syncing: Attempted to kill init! > exitcode=0x000b > > > All code > >0:51 push %rcx >1:50 push %rax >2:57 push %rdi >3:56 push %rsi >4:52 push %rdx >5:51 push %rcx >6:6a dapushq $0xffda >8:41 50push %r8 >a:41 51push %r9 >c:41 52push %r10 >e:41 53push %r11 > 10:48 83 ec 30 sub$0x30,%rsp > 14:65 4c 8b 1c 25 c0 d2 mov%gs:0xd2c0,%r11 > 1b:00 00 > 1d:41 f7 03 df 39 08 90 testl $0x900839df,(%r11) > 24:0f 85 a5 00 00 00jne0xcf > 2a:50 push %rax > 2b:*ff 15 9c 95 d0 ffcallq *-0x2f6a64(%rip)# > 0xffd095cd<-- trapping instruction > 31:58 pop%rax > 32:48 3d 4c 01 00 00cmp$0x14c,%rax > 38:77 0fja 0x49 > 3a:4c 89 d1 mov%r10,%rcx > 3d:ff .byte 0xff > 3e:14 c5adc$0xc5,%al > > > so the original 'cli' was replaced with the pv call but to me the offset > looks a bit off, no? Shouldn't it always be positive? callq takes a 32bit signed displacement, so jumping back by up to 2G is perfectly legitimate. The #GP[0] however means that whatever 8 byte value was found at -0x2f6a64(%rip) was a non-canonical address. One option is that the pvops structure hasn't been initialised properly, but an alternative is that the relocation wasn't processed correctly, and the code is trying to reference something which isn't a function pointer. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH v2 4/9] x86/mm/tlb: Flush remote and local TLBs concurrently
On 03/07/2019 18:02, Nadav Amit wrote: >> On Jul 3, 2019, at 7:04 AM, Juergen Gross wrote: >> >> On 03.07.19 01:51, Nadav Amit wrote: >>> To improve TLB shootdown performance, flush the remote and local TLBs >>> concurrently. Introduce flush_tlb_multi() that does so. Introduce >>> paravirtual versions of flush_tlb_multi() for KVM, Xen and hyper-v (Xen >>> and hyper-v are only compile-tested). >>> While the updated smp infrastructure is capable of running a function on >>> a single local core, it is not optimized for this case. The multiple >>> function calls and the indirect branch introduce some overhead, and >>> might make local TLB flushes slower than they were before the recent >>> changes. >>> Before calling the SMP infrastructure, check if only a local TLB flush >>> is needed to restore the lost performance in this common case. This >>> requires to check mm_cpumask() one more time, but unless this mask is >>> updated very frequently, this should impact performance negatively. >>> Cc: "K. Y. Srinivasan" >>> Cc: Haiyang Zhang >>> Cc: Stephen Hemminger >>> Cc: Sasha Levin >>> Cc: Thomas Gleixner >>> Cc: Ingo Molnar >>> Cc: Borislav Petkov >>> Cc: x...@kernel.org >>> Cc: Juergen Gross >>> Cc: Paolo Bonzini >>> Cc: Dave Hansen >>> Cc: Andy Lutomirski >>> Cc: Peter Zijlstra >>> Cc: Boris Ostrovsky >>> Cc: linux-hyp...@vger.kernel.org >>> Cc: linux-ker...@vger.kernel.org >>> Cc: virtualization@lists.linux-foundation.org >>> Cc: k...@vger.kernel.org >>> Cc: xen-de...@lists.xenproject.org >>> Signed-off-by: Nadav Amit >>> --- >>> arch/x86/hyperv/mmu.c | 13 +++--- >>> arch/x86/include/asm/paravirt.h | 6 +-- >>> arch/x86/include/asm/paravirt_types.h | 4 +- >>> arch/x86/include/asm/tlbflush.h | 9 ++-- >>> arch/x86/include/asm/trace/hyperv.h | 2 +- >>> arch/x86/kernel/kvm.c | 11 +++-- >>> arch/x86/kernel/paravirt.c| 2 +- >>> arch/x86/mm/tlb.c | 65 --- >>> arch/x86/xen/mmu_pv.c | 20 ++--- >>> include/trace/events/xen.h| 2 +- >>> 10 files changed, 91 insertions(+), 43 deletions(-) >> ... >> >>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c >>> index beb44e22afdf..19e481e6e904 100644 >>> --- a/arch/x86/xen/mmu_pv.c >>> +++ b/arch/x86/xen/mmu_pv.c >>> @@ -1355,8 +1355,8 @@ static void xen_flush_tlb_one_user(unsigned long addr) >>> preempt_enable(); >>> } >>> -static void xen_flush_tlb_others(const struct cpumask *cpus, >>> -const struct flush_tlb_info *info) >>> +static void xen_flush_tlb_multi(const struct cpumask *cpus, >>> + const struct flush_tlb_info *info) >>> { >>> struct { >>> struct mmuext_op op; >>> @@ -1366,7 +1366,7 @@ static void xen_flush_tlb_others(const struct cpumask >>> *cpus, >>> const size_t mc_entry_size = sizeof(args->op) + >>> sizeof(args->mask[0]) * BITS_TO_LONGS(num_possible_cpus()); >>> - trace_xen_mmu_flush_tlb_others(cpus, info->mm, info->start, info->end); >>> + trace_xen_mmu_flush_tlb_multi(cpus, info->mm, info->start, info->end); >>> if (cpumask_empty(cpus)) >>> return; /* nothing to do */ >>> @@ -1375,9 +1375,17 @@ static void xen_flush_tlb_others(const struct >>> cpumask *cpus, >>> args = mcs.args; >>> args->op.arg2.vcpumask = to_cpumask(args->mask); >>> - /* Remove us, and any offline CPUS. */ >>> + /* Flush locally if needed and remove us */ >>> + if (cpumask_test_cpu(smp_processor_id(), to_cpumask(args->mask))) { >>> + local_irq_disable(); >>> + flush_tlb_func_local(info); >> I think this isn't the correct function for PV guests. >> >> In fact it should be much easier: just don't clear the own cpu from the >> mask, that's all what's needed. The hypervisor is just fine having the >> current cpu in the mask and it will do the right thing. > Thanks. I will do so in v3. I don’t think Hyper-V people would want to do > the same, unfortunately, since it would induce VM-exit on TLB flushes. Why do you believe the vmexit matters? You're talking one anyway for the IPI. Intel only have virtualised self-IPI, and while AMD do have working non-self IPIs, you still take a vmexit anyway if any destination vcpu isn't currently running in non-root mode (IIRC). At that point, you might as well have the hypervisor do all the hard work via a multi-cpu shootdown/flush hypercall, rather than trying to arrange it locally. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] x86/paravirt: Drop {read,write}_cr8() hooks
There is a lot of infrastructure for functionality which is used exclusively in __{save,restore}_processor_state() on the suspend/resume path. cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored independently by lapic_{suspend,resume}(). Delete the saving and restoration of cr8, which allows for the removal of both PVOPS. Signed-off-by: Andrew Cooper --- CC: x...@kernel.org CC: virtualization@lists.linux-foundation.org CC: Borislav Petkov CC: Peter Zijlstra CC: Andy Lutomirski CC: Nadav Amit CC: Stephane Eranian CC: Feng Tang CC: Juergen Gross CC: Boris Ostrovsky CC: Alok Kataria CC: "Rafael J. Wysocki" CC: Pavel Machek Spotted while reviewing "x86/apic: Initialize TPR to block interrupts 16-31" https://lore.kernel.org/lkml/dc04a9f8b234d7b0956a8d2560b8945bcd9c4bf7.1563117760.git.l...@kernel.org/ --- arch/x86/include/asm/paravirt.h | 12 arch/x86/include/asm/paravirt_types.h | 5 - arch/x86/include/asm/special_insns.h | 24 arch/x86/kernel/paravirt.c| 4 arch/x86/power/cpu.c | 4 arch/x86/xen/enlighten_pv.c | 15 --- 6 files changed, 64 deletions(-) diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h index c25c38a05c1c..0e4a0539c353 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -139,18 +139,6 @@ static inline void __write_cr4(unsigned long x) PVOP_VCALL1(cpu.write_cr4, x); } -#ifdef CONFIG_X86_64 -static inline unsigned long read_cr8(void) -{ - return PVOP_CALL0(unsigned long, cpu.read_cr8); -} - -static inline void write_cr8(unsigned long x) -{ - PVOP_VCALL1(cpu.write_cr8, x); -} -#endif - static inline void arch_safe_halt(void) { PVOP_VCALL0(irq.safe_halt); diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index 946f8f1f1efc..3c775fb5524b 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -119,11 +119,6 @@ struct pv_cpu_ops { void (*write_cr4)(unsigned long); -#ifdef CONFIG_X86_64 - unsigned long (*read_cr8)(void); - void (*write_cr8)(unsigned long); -#endif - /* Segment descriptor handling */ void (*load_tr_desc)(void); void (*load_gdt)(const struct desc_ptr *); diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 219be88a59d2..6d37b8fcfc77 100644 --- a/arch/x86/include/asm/special_insns.h +++ b/arch/x86/include/asm/special_insns.h @@ -73,20 +73,6 @@ static inline unsigned long native_read_cr4(void) void native_write_cr4(unsigned long val); -#ifdef CONFIG_X86_64 -static inline unsigned long native_read_cr8(void) -{ - unsigned long cr8; - asm volatile("movq %%cr8,%0" : "=r" (cr8)); - return cr8; -} - -static inline void native_write_cr8(unsigned long val) -{ - asm volatile("movq %0,%%cr8" :: "r" (val) : "memory"); -} -#endif - #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS static inline u32 rdpkru(void) { @@ -200,16 +186,6 @@ static inline void wbinvd(void) #ifdef CONFIG_X86_64 -static inline unsigned long read_cr8(void) -{ - return native_read_cr8(); -} - -static inline void write_cr8(unsigned long x) -{ - native_write_cr8(x); -} - static inline void load_gs_index(unsigned selector) { native_load_gs_index(selector); diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index 98039d7fb998..de4d4e8a54c1 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -311,10 +311,6 @@ struct paravirt_patch_template pv_ops = { .cpu.read_cr0 = native_read_cr0, .cpu.write_cr0 = native_write_cr0, .cpu.write_cr4 = native_write_cr4, -#ifdef CONFIG_X86_64 - .cpu.read_cr8 = native_read_cr8, - .cpu.write_cr8 = native_write_cr8, -#endif .cpu.wbinvd = native_wbinvd, .cpu.read_msr = native_read_msr, .cpu.write_msr = native_write_msr, diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c index 24b079e94bc2..1c58d8982728 100644 --- a/arch/x86/power/cpu.c +++ b/arch/x86/power/cpu.c @@ -122,9 +122,6 @@ static void __save_processor_state(struct saved_context *ctxt) ctxt->cr2 = read_cr2(); ctxt->cr3 = __read_cr3(); ctxt->cr4 = __read_cr4(); -#ifdef CONFIG_X86_64 - ctxt->cr8 = read_cr8(); -#endif ctxt->misc_enable_saved = !rdmsrl_safe(MSR_IA32_MISC_ENABLE, &ctxt->misc_enable); msr_save_context(ctxt); @@ -207,7 +204,6 @@ static void notrace __restore_processor_state(struct saved_context *ctxt) #else /* CONFIG X86_64 */ wrmsrl(MSR_EFER, ctxt->efer); - write_cr8(ctxt->cr8
Re: [PATCH] x86/paravirt: Drop {read,write}_cr8() hooks
On 15/07/2019 15:52, Juergen Gross wrote: > On 15.07.19 16:26, Andy Lutomirski wrote: >> On Mon, Jul 15, 2019 at 6:23 AM Juergen Gross wrote: >>> >>> On 15.07.19 15:00, Andrew Cooper wrote: >>>> There is a lot of infrastructure for functionality which is used >>>> exclusively in __{save,restore}_processor_state() on the >>>> suspend/resume >>>> path. >>>> >>>> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored >>>> independently by lapic_{suspend,resume}(). >>> >>> Aren't those called only with CONFIG_PM set? >>> >> >> >> Unless I'm missing something, we only build any of the restore code >> (including the write_cr8() call) if CONFIG_PM_SLEEP is set, and that >> selects CONFIG_PM, so we should be fine, I think. >> > > Okay, in that case I'd suggest to remove "cr8" from struct saved_context > as it won't be used any longer. To be honest, saving and restoring of cr8 without the rest of the Local APIC state is conceptually broken to begin with. If there are any bugs revealed by this, then the correct fixes are elsewhere. You're right about the saved_context. I'll submit a v2 with an even larger negative diffstat. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] x86/paravirt: Drop {read,write}_cr8() hooks
There is a lot of infrastructure for functionality which is used exclusively in __{save,restore}_processor_state() on the suspend/resume path. cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored by lapic_{suspend,resume}(). Saving and restoring cr8 independently of the rest of the Local APIC state isn't a clever thing to be doing. Delete the suspend/resume cr8 handling, which shrinks the size of struct saved_context, and allows for the removal of both PVOPS. Signed-off-by: Andrew Cooper --- CC: x...@kernel.org CC: virtualization@lists.linux-foundation.org CC: Borislav Petkov CC: Peter Zijlstra CC: Andy Lutomirski CC: Nadav Amit CC: Stephane Eranian CC: Feng Tang CC: Juergen Gross CC: Boris Ostrovsky CC: "Rafael J. Wysocki" CC: Pavel Machek Spotted while reviewing "x86/apic: Initialize TPR to block interrupts 16-31" https://lore.kernel.org/lkml/dc04a9f8b234d7b0956a8d2560b8945bcd9c4bf7.1563117760.git.l...@kernel.org/ v2: * Drop saved_context.cr8 as well (Juergen) * Remove akata...@vmware.com from the CC list due to bounces --- arch/x86/include/asm/paravirt.h | 12 arch/x86/include/asm/paravirt_types.h | 5 - arch/x86/include/asm/special_insns.h | 24 arch/x86/include/asm/suspend_64.h | 2 +- arch/x86/kernel/asm-offsets_64.c | 1 - arch/x86/kernel/paravirt.c| 4 arch/x86/power/cpu.c | 4 arch/x86/xen/enlighten_pv.c | 15 --- 8 files changed, 1 insertion(+), 66 deletions(-) diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h index c25c38a05c1c..0e4a0539c353 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -139,18 +139,6 @@ static inline void __write_cr4(unsigned long x) PVOP_VCALL1(cpu.write_cr4, x); } -#ifdef CONFIG_X86_64 -static inline unsigned long read_cr8(void) -{ - return PVOP_CALL0(unsigned long, cpu.read_cr8); -} - -static inline void write_cr8(unsigned long x) -{ - PVOP_VCALL1(cpu.write_cr8, x); -} -#endif - static inline void arch_safe_halt(void) { PVOP_VCALL0(irq.safe_halt); diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index 946f8f1f1efc..3c775fb5524b 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -119,11 +119,6 @@ struct pv_cpu_ops { void (*write_cr4)(unsigned long); -#ifdef CONFIG_X86_64 - unsigned long (*read_cr8)(void); - void (*write_cr8)(unsigned long); -#endif - /* Segment descriptor handling */ void (*load_tr_desc)(void); void (*load_gdt)(const struct desc_ptr *); diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 219be88a59d2..6d37b8fcfc77 100644 --- a/arch/x86/include/asm/special_insns.h +++ b/arch/x86/include/asm/special_insns.h @@ -73,20 +73,6 @@ static inline unsigned long native_read_cr4(void) void native_write_cr4(unsigned long val); -#ifdef CONFIG_X86_64 -static inline unsigned long native_read_cr8(void) -{ - unsigned long cr8; - asm volatile("movq %%cr8,%0" : "=r" (cr8)); - return cr8; -} - -static inline void native_write_cr8(unsigned long val) -{ - asm volatile("movq %0,%%cr8" :: "r" (val) : "memory"); -} -#endif - #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS static inline u32 rdpkru(void) { @@ -200,16 +186,6 @@ static inline void wbinvd(void) #ifdef CONFIG_X86_64 -static inline unsigned long read_cr8(void) -{ - return native_read_cr8(); -} - -static inline void write_cr8(unsigned long x) -{ - native_write_cr8(x); -} - static inline void load_gs_index(unsigned selector) { native_load_gs_index(selector); diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h index a7af9f53c0cb..35bb35d28733 100644 --- a/arch/x86/include/asm/suspend_64.h +++ b/arch/x86/include/asm/suspend_64.h @@ -34,7 +34,7 @@ struct saved_context { */ unsigned long kernelmode_gs_base, usermode_gs_base, fs_base; - unsigned long cr0, cr2, cr3, cr4, cr8; + unsigned long cr0, cr2, cr3, cr4; u64 misc_enable; bool misc_enable_saved; struct saved_msrs saved_msrs; diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c index d3d075226c0a..8b54d8e3a561 100644 --- a/arch/x86/kernel/asm-offsets_64.c +++ b/arch/x86/kernel/asm-offsets_64.c @@ -62,7 +62,6 @@ int main(void) ENTRY(cr2); ENTRY(cr3); ENTRY(cr4); - ENTRY(cr8); ENTRY(gdt_desc); BLANK(); #undef ENTRY diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index 98039d7fb998..de4d4e8a54c1 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -311,10 +311,6 @@ struct paravirt_patch_template pv_ops =
Re: [Xen-devel] [PATCH 0/2] Remove 32-bit Xen PV guest support
On 15/07/2019 18:28, Andy Lutomirski wrote: > On Mon, Jul 15, 2019 at 9:34 AM Andi Kleen wrote: >> Juergen Gross writes: >> >>> The long term plan has been to replace Xen PV guests by PVH. The first >>> victim of that plan are now 32-bit PV guests, as those are used only >>> rather seldom these days. Xen on x86 requires 64-bit support and with >>> Grub2 now supporting PVH officially since version 2.04 there is no >>> need to keep 32-bit PV guest support alive in the Linux kernel. >>> Additionally Meltdown mitigation is not available in the kernel running >>> as 32-bit PV guest, so dropping this mode makes sense from security >>> point of view, too. >> Normally we have a deprecation period for feature removals like this. >> You would make the kernel print a warning for some releases, and when >> no user complains you can then remove. If a user complains you can't. >> > As I understand it, the kernel rules do allow changes like this even > if there's a complaint: this is a patch that removes what is > effectively hardware support. If the maintenance cost exceeds the > value, then removal is fair game. (Obviously we weight the value to > preserving compatibility quite highly, but in this case, Xen dropped > 32-bit hardware support a long time ago. If the Xen hypervisor says > that 32-bit PV guest support is deprecated, it's deprecated.) > > That being said, a warning might not be a bad idea. What's the > current status of this in upstream Xen? So personally, I'd prefer to see support stay, but at the end of the day it is Juergen's choice as the maintainer of the code. Xen itself has been exclusively 64-bit since Xen 4.3 (released in 2013). Over time, various features like SMEP/SMAP have been making 32bit PV guests progressively slower, because ring 1 is supervisor rather than user. Things have got even worse with IBRS, to the point at which 32bit PV guests are starting to run like treacle. There are no current plans to remove support for 32bit PV guests from Xen, but it is very much in the category of "you shouldn't be using this mode any more". ~Andrew P.S. I don't see 64bit PV guest support going anywhere, because there are still a number of open performance questions due to the inherent differences between syscall and vmexit, and the difference EPT/NPT tables make on cross-domain mappings. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] x86/paravirt: Drop {read,write}_cr8() hooks
On 15/07/2019 19:17, Nadav Amit wrote: >> On Jul 15, 2019, at 8:16 AM, Andrew Cooper wrote: >> >> There is a lot of infrastructure for functionality which is used >> exclusively in __{save,restore}_processor_state() on the suspend/resume >> path. >> >> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored by >> lapic_{suspend,resume}(). Saving and restoring cr8 independently of the >> rest of the Local APIC state isn't a clever thing to be doing. >> >> Delete the suspend/resume cr8 handling, which shrinks the size of struct >> saved_context, and allows for the removal of both PVOPS. > I think removing the interface for CR8 writes is also good to avoid > potential correctness issues, as the SDM says (10.8.6.1 "Interaction of Task > Priorities between CR8 and APIC”): > > "Operating software should implement either direct APIC TPR updates or CR8 > style TPR updates but not mix them. Software can use a serializing > instruction (for example, CPUID) to serialize updates between MOV CR8 and > stores to the APIC.” > > And native_write_cr8() did not even issue a serializing instruction. > Given its location, the one write_cr8() is bounded by two serialising operations, so is safe in practice. However, I agree with the statement in the manual. I could submit a v3 with an updated commit message, or let it be fixed on commit. Whichever is easiest. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] x86/paravirt: Drop {read,write}_cr8() hooks
On 16/07/2019 01:05, Andy Lutomirski wrote: > On Mon, Jul 15, 2019 at 4:30 PM Andrew Cooper > wrote: >> On 15/07/2019 19:17, Nadav Amit wrote: >>>> On Jul 15, 2019, at 8:16 AM, Andrew Cooper >>>> wrote: >>>> >>>> There is a lot of infrastructure for functionality which is used >>>> exclusively in __{save,restore}_processor_state() on the suspend/resume >>>> path. >>>> >>>> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored by >>>> lapic_{suspend,resume}(). Saving and restoring cr8 independently of the >>>> rest of the Local APIC state isn't a clever thing to be doing. >>>> >>>> Delete the suspend/resume cr8 handling, which shrinks the size of struct >>>> saved_context, and allows for the removal of both PVOPS. >>> I think removing the interface for CR8 writes is also good to avoid >>> potential correctness issues, as the SDM says (10.8.6.1 "Interaction of Task >>> Priorities between CR8 and APIC”): >>> >>> "Operating software should implement either direct APIC TPR updates or CR8 >>> style TPR updates but not mix them. Software can use a serializing >>> instruction (for example, CPUID) to serialize updates between MOV CR8 and >>> stores to the APIC.” >>> >>> And native_write_cr8() did not even issue a serializing instruction. >>> >> Given its location, the one write_cr8() is bounded by two serialising >> operations, so is safe in practice. >> >> However, I agree with the statement in the manual. I could submit a v3 >> with an updated commit message, or let it be fixed on commit. Whichever >> is easiest. >> > I don't see anything wrong with the message. If we actually used CR8 > for interrupt priorities, we wouldn't want it to serialize. The bug > is that the code that did the write_cr8() should have had a comment as > to how it serialized against lapic_restore(). But that doesn't seem > worth mentioning in the message, since, as noted, the real problem was > that it nonsensically restored just TPR without restoring everything > else. Fair enough, in which case I'm happy with v2 as it is. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 27/04/2020 18:37, Andy Lutomirski wrote: > On Sat, Apr 25, 2020 at 3:10 PM Andy Lutomirski wrote: >> On Sat, Apr 25, 2020 at 1:23 PM Joerg Roedel wrote: >>> On Sat, Apr 25, 2020 at 12:47:31PM -0700, Andy Lutomirski wrote: I assume the race you mean is: #VC Immediate NMI before IST gets shifted #VC Kaboom. How are you dealing with this? Ultimately, I think that NMI will need to turn off IST before engaging in any funny business. Let me ponder this a bit. >>> Right, I dealt with that by unconditionally shifting/unshifting the #VC IST >>> entry >>> in do_nmi() (thanks to Davin Kaplan for the idea). It might cause >>> one of the IST stacks to be unused during nesting, but that is fine. The >>> stack memory for #VC is only allocated when SEV-ES is active (in an >>> SEV-ES VM). >> Blech. It probably works, but still, yuck. It's a bit sad that we >> seem to be growing more and more poorly designed happens-anywhere >> exception types at an alarming rate. We seem to have #NM, #MC, #VC, >> #HV, and #DB. This doesn't really scale. > I have a somewhat serious question: should we use IST for #VC at all? > As I understand it, Rome and Naples make it mandatory for hypervisors > to intercept #DB, which means that, due to the MOV SS mess, it's sort > of mandatory to use IST for #VC. But Milan fixes the #DB issue, so, > if we're running under a sufficiently sensible hypervisor, we don't > need IST for #VC. > > So I think we have two choices: > > 1. Use IST for #VC and deal with all the mess that entails. > > 2. Say that we SEV-ES client support on Rome and Naples is for > development only and do a quick boot-time check for whether #DB is > intercepted. (Just set TF and see what vector we get.) If #DB is > intercepted, print a very loud warning and refuse to boot unless some > special sev_es.insecure_development_mode or similar option is set. > > #2 results in simpler and more robust entry code. #1 is more secure. > > So my question is: will anyone actually use SEV-ES in production on > Rome or Naples? As I understand it, it's not really ready for prime > time on those chips. And do we care if the combination of a malicious > hypervisor and malicious guest userspace on Milan can compromise the > guest kernel? I don't think SEV-ES is really mean to resist a > concerted effort by the hypervisor to compromise the guest. More specifically, it is mandatory for hypervisors to intercept #DB to defend against CVE-2015-8104, unless they're willing to trust the guest not to tickle that corner case. This is believed fixed with SEV-SNP to allow the encrypted guest to use debugging functionality without posing a DoS risk to the host. In this case, the hypervisor is expected not to intercept #DB. If #DB is intercepted, and #VC doesn't use IST, malicious userspace can cause problems with a movss-delayed breakpoint over SYSCALL. The question basically whether it is worth going to the effort of making #VC IST and all the problems that entails, to cover one corner case in earlier hardware. Ultimately, this depends on whether anyone plans to put SEV-ES into production on pre SEV-SNP hardware, and if developers using pre-SEV-SNP hardware are happy with "don't run malicious userspace" or "don't run malicious kernels and skip the #DB intercept" as a fair tradeoff to avoid the #VC IST fun. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 28/04/2020 08:55, Joerg Roedel wrote: > On Mon, Apr 27, 2020 at 10:37:41AM -0700, Andy Lutomirski wrote: >> I have a somewhat serious question: should we use IST for #VC at all? >> As I understand it, Rome and Naples make it mandatory for hypervisors >> to intercept #DB, which means that, due to the MOV SS mess, it's sort >> of mandatory to use IST for #VC. But Milan fixes the #DB issue, so, >> if we're running under a sufficiently sensible hypervisor, we don't >> need IST for #VC. > The reason for #VC being IST is not only #DB, but also SEV-SNP. SNP adds > page ownership tracking between guest and host, so that the hypervisor > can't remap guest pages without the guest noticing. > > If there is a violation of ownership, which can happen at any memory > access, there will be a #VC exception to notify the guest. And as this > can happen anywhere, for example on a carefully crafted stack page set > by userspace before doing SYSCALL, the only robust choice for #VC is to > use IST. The kernel won't ever touch the guest stack before restoring %rsp in the syscall path, but the (minimum 2) memory accesses required to save the user %rsp and load the kernel stack may be subject to #VC exceptions, as are instruction fetches at the head of the SYSCALL path. So yes - #VC needs IST. Sorry for the noise. (That said, it is unfortunate that the hypervisor messing with the memory backing the guest #VC handler results in an infinite loop, rather than an ability to cleanly terminate.) ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 23/06/2020 12:30, Joerg Roedel wrote: > On Tue, Jun 23, 2020 at 01:07:06PM +0200, Peter Zijlstra wrote: >> On Tue, Apr 28, 2020 at 09:55:12AM +0200, Joerg Roedel wrote: >> So what happens if this #VC triggers on the first access to the #VC >> stack, because the malicious host has craftily mucked with only the #VC >> IST stack page? >> >> Or on the NMI IST stack, then we get #VC in NMI before the NMI can fix >> you up. >> >> AFAICT all of that is non-recoverable. > I am not 100% sure, but I think if the #VC stack page is not validated, > the #VC should be promoted to a #DF. > > Note that this is an issue only with secure nested paging (SNP), which > is not enabled yet with this patch-set. When it gets enabled a stack > recursion check in the #VC handler is needed which panics the VM. That > also fixes the #VC-in-early-NMI problem. There are cases which are definitely non-recoverable. For both ES and SNP, a malicious hypervisor can mess with the guest physmap to make the the NMI, #VC and #DF stacks all alias. For ES, this had better result in the #DF handler deciding that crashing is the way out, whereas for SNP, this had better escalate to Shutdown. What matters here is the security model in SNP. The hypervisor is relied upon for availability (because it could simply refuse to schedule the VM), but market/business forces will cause it to do its best to keep the VM running. Therefore, the securely model is simply(?) that the hypervisor can't do anything to undermine the confidentiality or integrity of the VM. Crashing out hard if the hypervisor is misbehaving is acceptable. In a cloud, I as a customer would (threaten to?) take my credit card elsewhere, while for enterprise, I'd shout at my virtualisation vendor until a fix happened (also perhaps threaten to take my credit card elsewhere). Therefore, it is reasonable to build on the expectation that the hypervisor won't be messing around with remapping stacks/etc. Most #VC's are synchronous with guest actions (they equate to actions which would have caused a VMExit), so you can safely reason about when the first #VC might occur, presuming no funny business with the frames backing any memory operands touched. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 23/06/2020 13:47, Peter Zijlstra wrote: > On Tue, Jun 23, 2020 at 12:51:03PM +0100, Andrew Cooper wrote: > >> There are cases which are definitely non-recoverable. >> >> For both ES and SNP, a malicious hypervisor can mess with the guest >> physmap to make the the NMI, #VC and #DF stacks all alias. >> >> For ES, this had better result in the #DF handler deciding that crashing >> is the way out, whereas for SNP, this had better escalate to Shutdown. >> Crashing out hard if the hypervisor is misbehaving is acceptable. > Then I'm thinking the only sensible option is to crash hard for any SNP > #VC from kernel mode. > > Sadly that doesn't help with #VC needing to be IST :-( IST is such a > frigging nightmare. I presume you mean any #VC caused by RMP faults (i.e. something went wrong with the memory owner/etc metadata) ? If so, then yes. Any failure here is a bug in the kernel or hypervisor (and needs fixing) or a malicious hypervisor and the guest should terminate for its own safety. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 23/06/2020 14:03, Peter Zijlstra wrote: > On Tue, Jun 23, 2020 at 02:12:37PM +0200, Joerg Roedel wrote: >> On Tue, Jun 23, 2020 at 01:50:14PM +0200, Peter Zijlstra wrote: >>> If SNP is the sole reason #VC needs to be IST, then I'd strongly urge >>> you to only make it IST if/when you try and make SNP happen, not before. >> It is not the only reason, when ES guests gain debug register support >> then #VC also needs to be IST, because #DB can be promoted into #VC >> then, and as #DB is IST for a reason, #VC needs to be too. > Didn't I read somewhere that that is only so for Rome/Naples but not for > the later chips (Milan) which have #DB pass-through? I don't know about hardware timelines, but some future part can now opt in to having debug registers as part of the encrypted state, and swapped by VMExit, which would make debug facilities generally usable, and supposedly safe to the #DB infinite loop issues, at which point the hypervisor need not intercept #DB for safety reasons. Its worth nothing that on current parts, the hypervisor can set up debug facilities on behalf of the guest (or behind its back) as the DR state is unencrypted, but that attempting to intercept #DB will redirect to #VC inside the guest and cause fun. (Also spare a thought for 32bit kernels which have to cope with userspace singlestepping the SYSENTER path with every #DB turning into #VC.) >> Besides that, I am not a fan of delegating problems I already see coming >> to future-Joerg and future-Peter, but if at all possible deal with them >> now and be safe later. > Well, we could just say no :-) At some point in the very near future > this house of cards is going to implode. What currently exists is a picture of a house of cards in front of something which has fallen down. > Did someone forget to pass the 'ISTs are *EVIL*' memo to the hardware > folks? How come we're getting more and more of them? I have tried to get this point across. Then again - its far easier for the software folk in the same company as the hardware folk to make this point. > (/me puts fingers > in ears and goes la-la-la-la in anticipation of Andrew mentioning CET) I wasn't going to bring it up, but seeing as you have - while there are prohibitively-complicating issues preventing it from working on native, I don't see any point even considering it for the mess which is #VC, or the even bigger mess which is #HV. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 23/06/2020 16:16, Peter Zijlstra wrote: > On Tue, Jun 23, 2020 at 04:49:40PM +0200, Joerg Roedel wrote: >>> We're talking about the 3rd case where the only reason things 'work' is >>> because we'll have to panic(): >>> >>> - #MC >> Okay, #MC is special and can only be handled on a best-effort basis, as >> #MC could happen anytime, also while already executing the #MC handler. > I think the hardware has a MCE-mask bit somewhere. Flaky though because > clearing it isn't 'atomic' with IRET, so there's a 'funny' window. MSR_MCG_STATUS.MCIP, and yes - any #MC before that point will immediately Shutdown. Any #MC between that point and IRET will clobber its IST stack and end up sad. > It also interacts really bad with the NMI handler. If we get an #MC > early in the NMI, where we hard-rely on the NMI-mask being set to set-up > the recursion stack, then the #MC IRET will clear the NMI-mask, and > we're toast. > > Andy has wild and crazy ideas, but I don't think we need more crazy > here. Want, certainly not. Need, maybe :-/ >>> - #DB with BUS LOCK DEBUG EXCEPTION >> If I understand the problem correctly, this can be solved by moving off >> the IST stack to the current task stack in the #DB handler, like I plan >> to do for #VC, no? > Hmm, probably. Would take a bit of care, but should be doable. Andy and I have spent some time considering this. Having exactly one vector move off IST and onto an in-use task-stack is doable, I think, so long as it can sort out self-recursion protections. Having more than one vector do this is very complicated. You've got to take care to walk up the list of IST-nesting to see if any interrupted context is in the middle of trying to copy themselves onto the stack, so you don't clobber the frame they're in the middle of copying. ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 23/06/2020 16:23, Peter Zijlstra wrote: > On Tue, Jun 23, 2020 at 04:59:14PM +0200, Joerg Roedel wrote: >> On Tue, Jun 23, 2020 at 04:53:44PM +0200, Peter Zijlstra wrote: >>> +noinstr void idtentry_validate_ist(struct pt_regs *regs) >>> +{ >>> + if ((regs->sp & ~(EXCEPTION_STKSZ-1)) == >>> + (_RET_IP_ & ~(EXCEPTION_STKSZ-1))) >>> + die("IST stack recursion", regs, 0); >>> +} >> Yes, this is a start, it doesn't cover the case where the NMI stack is >> in-between, so I think you need to walk down regs->sp too. > That shouldn't be possible with the current code, I think. NMI; #MC; Anything which IRET but isn't fatal - #DB, or #BP from patching, #GP from *_safe(), etc; NMI Sure its a corner case, but did you hear that IST is evil? ~Andrew P.S. did you also hear that with Rowhammer, userspace has a nonzero quantity of control over generating #MC, depending on how ECC is configured on the platform. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)
On 23/06/2020 19:26, Andy Lutomirski wrote: > On Tue, Jun 23, 2020 at 8:23 AM Andrew Cooper > wrote: >> On 23/06/2020 14:03, Peter Zijlstra wrote: >>> On Tue, Jun 23, 2020 at 02:12:37PM +0200, Joerg Roedel wrote: >>>> On Tue, Jun 23, 2020 at 01:50:14PM +0200, Peter Zijlstra wrote: >>>>> If SNP is the sole reason #VC needs to be IST, then I'd strongly urge >>>>> you to only make it IST if/when you try and make SNP happen, not before. >>>> It is not the only reason, when ES guests gain debug register support >>>> then #VC also needs to be IST, because #DB can be promoted into #VC >>>> then, and as #DB is IST for a reason, #VC needs to be too. >>> Didn't I read somewhere that that is only so for Rome/Naples but not for >>> the later chips (Milan) which have #DB pass-through? >> I don't know about hardware timelines, but some future part can now opt >> in to having debug registers as part of the encrypted state, and swapped >> by VMExit, which would make debug facilities generally usable, and >> supposedly safe to the #DB infinite loop issues, at which point the >> hypervisor need not intercept #DB for safety reasons. >> >> Its worth nothing that on current parts, the hypervisor can set up debug >> facilities on behalf of the guest (or behind its back) as the DR state >> is unencrypted, but that attempting to intercept #DB will redirect to >> #VC inside the guest and cause fun. (Also spare a thought for 32bit >> kernels which have to cope with userspace singlestepping the SYSENTER >> path with every #DB turning into #VC.) > What do you mean 32-bit? 64-bit kernels have exactly the same > problem. At least the stack is okay, though. :) AMD-like CPUs disallow SYSENTER/SYSEXIT in Long Mode, and raise #UD, even from a compatibility mode segment. 64bit kernels only have this problem on Intel-like CPUs. (It is a massive shame that between everyone's attempts, there are 0 "fast system call" instructions with sane semantics, but it is several decades late to fix this problem...) ~Andrew ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization