| Message ID | 1e221192-7899-b60d-0280-b77ab5877772@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: mm (mainly shadow) adjustments | expand |
At 16:28 +0200 on 17 Apr (1587140897), Jan Beulich wrote: > sh_remove_write_access() bails early for !external guests, and hence its > building and thus the need for the hook can be suppressed altogether in > !HVM configs. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > @@ -366,6 +367,14 @@ int sh_validate_guest_entry(struct vcpu > extern int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, > unsigned int level, > unsigned long fault_addr); > +#else > +static inline int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, > + unsigned int level, > + unsigned long fault_addr) > +{ Can we have an ASSERT(!shadow_mode_refcounts(d)) here, please, matching the check that would have made it a noop before? Cheers, Tim.
On 18.04.2020 11:03, Tim Deegan wrote: > At 16:28 +0200 on 17 Apr (1587140897), Jan Beulich wrote: >> sh_remove_write_access() bails early for !external guests, and hence its >> building and thus the need for the hook can be suppressed altogether in >> !HVM configs. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> @@ -366,6 +367,14 @@ int sh_validate_guest_entry(struct vcpu >> extern int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, >> unsigned int level, >> unsigned long fault_addr); >> +#else >> +static inline int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, >> + unsigned int level, >> + unsigned long fault_addr) >> +{ > > Can we have an ASSERT(!shadow_mode_refcounts(d)) here, please, > matching the check that would have made it a noop before? I've added one, but I find this quite odd in a !HVM build, where #define PG_refcounts 0 and #define paging_mode_refcounts(_d) (!!((_d)->arch.paging.mode & PG_refcounts)) Perhaps you're wanting this mainly for documentation purposes? Jan
At 15:06 +0200 on 20 Apr (1587395210), Jan Beulich wrote: > On 18.04.2020 11:03, Tim Deegan wrote: > > At 16:28 +0200 on 17 Apr (1587140897), Jan Beulich wrote: > >> sh_remove_write_access() bails early for !external guests, and hence its > >> building and thus the need for the hook can be suppressed altogether in > >> !HVM configs. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > >> @@ -366,6 +367,14 @@ int sh_validate_guest_entry(struct vcpu > >> extern int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, > >> unsigned int level, > >> unsigned long fault_addr); > >> +#else > >> +static inline int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, > >> + unsigned int level, > >> + unsigned long fault_addr) > >> +{ > > > > Can we have an ASSERT(!shadow_mode_refcounts(d)) here, please, > > matching the check that would have made it a noop before? > > I've added one, but I find this quite odd in a !HVM build, where > > #define PG_refcounts 0 > > and > > #define paging_mode_refcounts(_d) (!!((_d)->arch.paging.mode & PG_refcounts)) > > Perhaps you're wanting this mainly for documentation purposes? Yeah, that and future-proofing. If !HVM builds ever start using paging_mode_refcounts then this assertion will forcibly remind us that we need changes here. I'm glad that it compiles away to nothing in the meantime. Cheers, Tim.
--- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -1769,6 +1769,7 @@ static inline void trace_shadow_wrmap_bf } } +#ifdef CONFIG_HVM /**************************************************************************/ /* Remove all writeable mappings of a guest frame from the shadow tables * Returns non-zero if we need to flush TLBs. @@ -2000,6 +2001,7 @@ int sh_remove_write_access(struct domain /* We killed at least one writeable mapping, so must flush TLBs. */ return 1; } +#endif /* CONFIG_HVM */ #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC) static int sh_remove_write_access_from_sl1p(struct domain *d, mfn_t gmfn, --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -4203,7 +4203,7 @@ int sh_rm_write_access_from_sl1p(struct } #endif /* OOS */ -#if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC +#if defined(CONFIG_HVM) && (SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC) static int sh_guess_wrmap(struct vcpu *v, unsigned long vaddr, mfn_t gmfn) /* Look up this vaddr in the current shadow and see if it's a writeable * mapping of this gmfn. If so, remove it. Returns 1 if it worked. */ @@ -4875,10 +4875,10 @@ const struct paging_mode sh_paging_mode #ifdef CONFIG_HVM .shadow.make_monitor_table = sh_make_monitor_table, .shadow.destroy_monitor_table = sh_destroy_monitor_table, -#endif #if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC .shadow.guess_wrmap = sh_guess_wrmap, #endif +#endif /* CONFIG_HVM */ .shadow.pagetable_dying = sh_pagetable_dying, .shadow.trace_emul_write_val = trace_emulate_write_val, .shadow.shadow_levels = SHADOW_PAGING_LEVELS, --- a/xen/arch/x86/mm/shadow/private.h +++ b/xen/arch/x86/mm/shadow/private.h @@ -359,6 +359,7 @@ void sh_install_xen_entries_in_l4(struct /* Update the shadows in response to a pagetable write from Xen */ int sh_validate_guest_entry(struct vcpu *v, mfn_t gmfn, void *entry, u32 size); +#ifdef CONFIG_HVM /* Remove all writeable mappings of a guest frame from the shadows. * Returns non-zero if we need to flush TLBs. * level and fault_addr desribe how we found this to be a pagetable; @@ -366,6 +367,14 @@ int sh_validate_guest_entry(struct vcpu extern int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, unsigned int level, unsigned long fault_addr); +#else +static inline int sh_remove_write_access(struct domain *d, mfn_t readonly_mfn, + unsigned int level, + unsigned long fault_addr) +{ + return 0; +} +#endif /* Functions that atomically write PT/P2M entries and update state */ int shadow_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, --- a/xen/include/asm-x86/paging.h +++ b/xen/include/asm-x86/paging.h @@ -105,9 +105,9 @@ struct shadow_paging_mode { #ifdef CONFIG_HVM mfn_t (*make_monitor_table )(struct vcpu *v); void (*destroy_monitor_table )(struct vcpu *v, mfn_t mmfn); -#endif int (*guess_wrmap )(struct vcpu *v, unsigned long vaddr, mfn_t gmfn); +#endif void (*pagetable_dying )(paddr_t gpa); void (*trace_emul_write_val )(const void *ptr, unsigned long vaddr, const void *src, unsigned int bytes);
sh_remove_write_access() bails early for !external guests, and hence its building and thus the need for the hook can be suppressed altogether in !HVM configs. Signed-off-by: Jan Beulich <jbeulich@suse.com>