|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC] xen/libxl/libxl: RIP PV superpage.
>>> On 29.01.16 at 16:30, <konrad.wilk@xxxxxxxxxx> wrote:
> @@ -985,20 +968,7 @@ get_page_from_l2e(
> return rc;
> }
>
> - if ( !opt_allow_superpage )
> - {
> - MEM_LOG("Attempt to map superpage without allowsuperpage "
> - "flag in hypervisor");
> - return -EINVAL;
> - }
> -
> - if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
> - {
> - MEM_LOG("Unaligned superpage map attempt mfn %lx", mfn);
> - return -EINVAL;
> - }
> -
> - return get_superpage(mfn, d);
> + return -EINVAL;
> }
This return is now unreachable. You should instead remove the if()
around the code block right ahead of the code you delete here,
which is possible/correct due to _PAGE_PSE now always being in
L2_DISALLOW_MASK.
> static void put_superpage(unsigned long mfn)
> {
> - struct spage_info *spage;
> - unsigned long x, nx, y;
> - unsigned long do_pages = 0;
> -
> - if ( !opt_allow_superpage )
> - {
> - put_spage_pages(mfn_to_page(mfn));
> - return;
> - }
> -
> - spage = mfn_to_spage(mfn);
> - y = spage->type_info;
> - do {
> - x = y;
> - nx = x - 1;
> - if ((x & SGT_type_mask) == SGT_dynamic)
> - {
> - if ((nx & SGT_count_mask) == 0)
> - {
> - nx = (nx & ~SGT_type_mask) | SGT_none;
> - do_pages = 1;
> - }
> - }
> -
> - } while ((y = cmpxchg(&spage->type_info, x, nx)) != x);
> -
> - if (do_pages)
> - put_spage_pages(spage_to_page(spage));
> -
> - return;
> + ASSERT(1); /* This should really not be called. */
> + put_spage_pages(mfn_to_page(mfn));
> }
As follows from the reply to your intro mail, this ASSERT needs to
go away. But very likely it would be best to inline the body of
put_spage_pages() into its only caller, to not needlessly leave
traces of this super-page code.
> @@ -3437,29 +3231,8 @@ long do_mmuext_op(
>
> case MMUEXT_MARK_SUPER:
> case MMUEXT_UNMARK_SUPER:
> - {
> - unsigned long mfn = op.arg1.mfn;
> -
> - if ( !opt_allow_superpage )
> - {
> - MEM_LOG("Superpages disallowed");
> - rc = -ENOSYS;
> - }
> - else if ( unlikely(d != pg_owner) )
> - rc = -EPERM;
> - else if ( mfn & (L1_PAGETABLE_ENTRIES - 1) )
> - {
> - MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
> - rc = -EINVAL;
> - }
> - else if ( !mfn_valid(mfn | (L1_PAGETABLE_ENTRIES - 1)) )
> - rc = -EINVAL;
> - else if ( op.cmd == MMUEXT_MARK_SUPER )
> - rc = mark_superpage(mfn_to_spage(mfn), d);
> - else
> - rc = unmark_superpage(mfn_to_spage(mfn));
> - break;
> - }
> + MEM_LOG("Superpages are deprecated.");
> + rc = -ENOSYS;
>
> default:
> MEM_LOG("Invalid extended pt command %#x", op.cmd);
I think having this handled by the default case would be good
enough. MEM_LOG() doesn't print anything in release builds
anyway, and for debug builds we don't need to do more than
the minimum here.
If you feel strongly about leaving the code in, you need to add
a break statement.
> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -239,13 +239,8 @@ extern unsigned char boot_edid_info[128];
> #define XEN_VIRT_START (HIRO_COMPAT_MPT_VIRT_END)
> #define XEN_VIRT_END (XEN_VIRT_START + GB(1))
>
> -/* Slot 261: superpage information array (64MB or 512MB). */
> -#define SPAGETABLE_VIRT_END FRAMETABLE_VIRT_START
> -#define SPAGETABLE_NR (((FRAMETABLE_NR - 1) >> (SUPERPAGE_SHIFT - \
> - PAGE_SHIFT)) + 1)
> -#define SPAGETABLE_SIZE (SPAGETABLE_NR * sizeof(struct spage_info))
> -#define SPAGETABLE_VIRT_START ((SPAGETABLE_VIRT_END - SPAGETABLE_SIZE) & \
> - (_AC(-1,UL) << SUPERPAGE_SHIFT))
> +/* Slot 261: superpage information array (64MB or 512MB).
> + * Now free. */
No, this is too simple a change. You should update more places
and not leave this kind of a comment around.
> --- a/xen/include/asm-x86/guest_pt.h
> +++ b/xen/include/asm-x86/guest_pt.h
> @@ -186,9 +186,7 @@ guest_supports_superpages(struct vcpu *v)
> /* The _PAGE_PSE bit must be honoured in HVM guests, whenever
> * CR4.PSE is set or the guest is in PAE or long mode.
> * It's also used in the dummy PT for vcpus with CR4.PG cleared. */
> - return (is_pv_vcpu(v)
> - ? opt_allow_superpage
> - : (GUEST_PAGING_LEVELS != 2
> + return (is_pv_vcpu(v) ? 0 /* Deprecated. */ : (GUEST_PAGING_LEVELS != 2
> || !hvm_paging_enabled(v)
> || (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PSE)));
> }
No need for this "Deprecated" comment.
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -272,7 +272,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
> * The one bits that does not fit with the default layout is the PAGE_PSE
> * also called PAGE_PAT). The MMUEXT_[UN]MARK_SUPER arguments to the
> * HYPERVISOR_mmuext_op serve as mechanism to set a pagetable to be 4MB
> - * (or 2MB) instead of using the PAGE_PSE bit.
> + * (or 2MB) instead of using the PAGE_PSE bit. That functionality is now
> + * deprecated and will return -ENOSYS.
> *
> * The reason that the PAGE_PSE (bit 7) is not being utilized is due to Xen
> * using it as the Page Attribute Table (PAT) bit - for details on it please
> @@ -382,6 +383,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
> *
> * cmd: MMUEXT_[UN]MARK_SUPER
> * mfn: Machine frame number of head of superpage to be [un]marked.
> + * These sub-ops are deprecated and will return -ENOSYS.
> */
"Deprecated" to means "do not use, but they still work".
Also you should be hiding their definitions inside an
#if __XEN_INTERFACE_VERSION__ < 0x00040700
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |