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: [PATCH v2] x86/paravirt: Drop {read,write}_cr8() hooks
On 15.07.19 17:16, 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. Signed-off-by: Andrew Cooper Reviewed-by: Juergen Gross Juergen ___ 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 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. That’s what the “potential” in "potential correctness issues” means :) ___ 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 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. ___ 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 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. ___ 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 = { .cpu.read_cr0 = native_read_cr0, .cpu.write_cr0