| Message ID | 1455818622-30625-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote: > It is not obvious what this code is doing. Most of it dates from 2007/2008, > and there have been substantial changes in Xen's memory handling since then. Deleting code which isn't understood what it is or was once used for is sub-optimal. > It was previously optional, and isn't needed for any of the memguard > infrastructure to function. The use of MAP_SMALL_PAGES causes needless > shattering of superpages. Perhaps that's what is its purpose? Let's ask Keir, whom you didn't even Cc. Jan
On 19/02/16 14:44, Jan Beulich wrote: >>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote: >> It is not obvious what this code is doing. Most of it dates from 2007/2008, >> and there have been substantial changes in Xen's memory handling since then. > Deleting code which isn't understood what it is or was once used > for is sub-optimal. > >> It was previously optional, and isn't needed for any of the memguard >> infrastructure to function. The use of MAP_SMALL_PAGES causes needless >> shattering of superpages. > Perhaps that's what is its purpose? Let's ask Keir, whom you didn't > even Cc. I don't see this patch being different in nature to your "x86: drop failsafe callback invocation from assembly". As I explain in the second paragraph, these calls are strictly optional, as they are omitted for release builds. They also have no impact on the rest of the memguard infrastructure to function, as __memguard_change_range() also uses map_pages_to_xen(). So despite not being sure why it is like it is, I am stating with that it is not needed with Xen in its current form. ~Andrew
>>> On 19.02.16 at 17:18, <andrew.cooper3@citrix.com> wrote: > On 19/02/16 14:44, Jan Beulich wrote: >>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote: >>> It is not obvious what this code is doing. Most of it dates from 2007/2008, >>> and there have been substantial changes in Xen's memory handling since then. >> Deleting code which isn't understood what it is or was once used >> for is sub-optimal. >> >>> It was previously optional, and isn't needed for any of the memguard >>> infrastructure to function. The use of MAP_SMALL_PAGES causes needless >>> shattering of superpages. >> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't >> even Cc. > > I don't see this patch being different in nature to your "x86: drop > failsafe callback invocation from assembly". That other patch explains why the code is (and never was) necessary, whereas you just guess. > As I explain in the second paragraph, these calls are strictly optional, > as they are omitted for release builds. They also have no impact on the > rest of the memguard infrastructure to function, as > __memguard_change_range() also uses map_pages_to_xen(). > > So despite not being sure why it is like it is, I am stating with that > it is not needed with Xen in its current form. I actually think the reason is to avoid the memory allocation which might result the first time a 2M page gets split up, as that memory allocation might fail (which nowadays gets a proper -ENOMEM communicated out of map_pages_to_xen(), but that hasn't been the case in the early days). Jan
On 22/02/16 10:02, Jan Beulich wrote: >>>> On 19.02.16 at 17:18, <andrew.cooper3@citrix.com> wrote: >> On 19/02/16 14:44, Jan Beulich wrote: >>>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote: >>>> It is not obvious what this code is doing. Most of it dates from 2007/2008, >>>> and there have been substantial changes in Xen's memory handling since then. >>> Deleting code which isn't understood what it is or was once used >>> for is sub-optimal. >>> >>>> It was previously optional, and isn't needed for any of the memguard >>>> infrastructure to function. The use of MAP_SMALL_PAGES causes needless >>>> shattering of superpages. >>> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't >>> even Cc. >> I don't see this patch being different in nature to your "x86: drop >> failsafe callback invocation from assembly". > That other patch explains why the code is (and never was) > necessary, whereas you just guess. > >> As I explain in the second paragraph, these calls are strictly optional, >> as they are omitted for release builds. They also have no impact on the >> rest of the memguard infrastructure to function, as >> __memguard_change_range() also uses map_pages_to_xen(). >> >> So despite not being sure why it is like it is, I am stating with that >> it is not needed with Xen in its current form. > I actually think the reason is to avoid the memory allocation which > might result the first time a 2M page gets split up, as that memory > allocation might fail (which nowadays gets a proper -ENOMEM > communicated out of map_pages_to_xen(), but that hasn't been > the case in the early days). And isn't the case anywhere in the memguard infrastructure. At the end of this series, there is no shattering of any superpages in the normal .text/data/bss region, but there is guarding of the pcpu stack which is liable to shatter specific superpages mapping the directmap region. ~Andrew
>>> On 22.02.16 at 11:29, <andrew.cooper3@citrix.com> wrote: > On 22/02/16 10:02, Jan Beulich wrote: >>>>> On 19.02.16 at 17:18, <andrew.cooper3@citrix.com> wrote: >>> On 19/02/16 14:44, Jan Beulich wrote: >>>>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote: >>>>> It is not obvious what this code is doing. Most of it dates from 2007/2008, >>>>> and there have been substantial changes in Xen's memory handling since then. >>>> Deleting code which isn't understood what it is or was once used >>>> for is sub-optimal. >>>> >>>>> It was previously optional, and isn't needed for any of the memguard >>>>> infrastructure to function. The use of MAP_SMALL_PAGES causes needless >>>>> shattering of superpages. >>>> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't >>>> even Cc. >>> I don't see this patch being different in nature to your "x86: drop >>> failsafe callback invocation from assembly". >> That other patch explains why the code is (and never was) >> necessary, whereas you just guess. >> >>> As I explain in the second paragraph, these calls are strictly optional, >>> as they are omitted for release builds. They also have no impact on the >>> rest of the memguard infrastructure to function, as >>> __memguard_change_range() also uses map_pages_to_xen(). >>> >>> So despite not being sure why it is like it is, I am stating with that >>> it is not needed with Xen in its current form. >> I actually think the reason is to avoid the memory allocation which >> might result the first time a 2M page gets split up, as that memory >> allocation might fail (which nowadays gets a proper -ENOMEM >> communicated out of map_pages_to_xen(), but that hasn't been >> the case in the early days). > > And isn't the case anywhere in the memguard infrastructure. > > At the end of this series, there is no shattering of any superpages in > the normal .text/data/bss region, but there is guarding of the pcpu > stack which is liable to shatter specific superpages mapping the > directmap region. In which case I guess I'm fine with the change, but I'd like you to re-word the commit message accordingly (in particular make it less vague). Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index d6aaed8..ed8ab02 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -6346,22 +6346,6 @@ void free_perdomain_mappings(struct domain *d) #ifdef MEMORY_GUARD -void memguard_init(void) -{ - unsigned long start = max_t(unsigned long, xen_phys_start, 1UL << 20); - map_pages_to_xen( - (unsigned long)__va(start), - start >> PAGE_SHIFT, - (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT, - __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES); - BUG_ON(start != xen_phys_start); - map_pages_to_xen( - XEN_VIRT_START, - start >> PAGE_SHIFT, - (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT, - __PAGE_HYPERVISOR|MAP_SMALL_PAGES); -} - static void __memguard_change_range(void *p, unsigned long l, int guard) { unsigned long _p = (unsigned long)p; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index b8a28d7..cddf954 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1146,8 +1146,6 @@ void __init noreturn __start_xen(unsigned long mbi_p) ~((1UL << L2_PAGETABLE_SHIFT) - 1); destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE); - memguard_init(); - nr_pages = 0; for ( i = 0; i < e820.nr_map; i++ ) if ( e820.map[i].type == E820_RAM ) diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index 2e9d0b2..68cf203 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -331,7 +331,6 @@ unsigned long domain_get_maximum_gpfn(struct domain *d); extern struct domain *dom_xen, *dom_io, *dom_cow; -#define memguard_init(_s) (_s) #define memguard_guard_stack(_p) ((void)0) #define memguard_guard_range(_p,_l) ((void)0) #define memguard_unguard_range(_p,_l) ((void)0) diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index a097382..23a4092 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -479,11 +479,9 @@ extern struct rangeset *mmio_ro_ranges; #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20)) #ifdef MEMORY_GUARD -void memguard_init(void); void memguard_guard_range(void *p, unsigned long l); void memguard_unguard_range(void *p, unsigned long l); #else -#define memguard_init() ((void)0) #define memguard_guard_range(_p,_l) ((void)0) #define memguard_unguard_range(_p,_l) ((void)0) #endif
It is not obvious what this code is doing. Most of it dates from 2007/2008, and there have been substantial changes in Xen's memory handling since then. It was previously optional, and isn't needed for any of the memguard infrastructure to function. The use of MAP_SMALL_PAGES causes needless shattering of superpages. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Ian Campbell <Ian.Campbell@citrix.com> --- xen/arch/x86/mm.c | 16 ---------------- xen/arch/x86/setup.c | 2 -- xen/include/asm-arm/mm.h | 1 - xen/include/asm-x86/mm.h | 2 -- 4 files changed, 21 deletions(-)