| Message ID | 20170823155824.11144-3-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 23, 2017 at 04:58:24PM +0100, Wei Liu wrote: > After 404595352 ("x86/paging: Enforce PG_external == PG_translate == > PG_refcounts"), PG_refcounts and PG_translate always need to be set > together. > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts > are replaced by calls to paging_mode_translate. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Tim Deegan <tim@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/arch/x86/domain.c | 6 ++---- > xen/arch/x86/hvm/hvm.c | 2 +- > xen/arch/x86/mm.c | 20 ++++++++++---------- > xen/arch/x86/mm/paging.c | 8 +++----- > xen/include/asm-x86/paging.h | 5 +---- > xen/include/asm-x86/shadow.h | 2 +- > 6 files changed, 18 insertions(+), 25 deletions(-) > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 9b4b9596d8..bbe545b165 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1017,8 +1017,6 @@ int arch_set_info_guest( > > if ( !cr3_page ) > rc = -EINVAL; > - else if ( paging_mode_refcounts(d) ) > - /* nothing */; It appears I accidentally deleted this hunk, but my tests were still happy... Given this function is rather convoluted I intend to add it back and deal with that later.
At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote: > After 404595352 ("x86/paging: Enforce PG_external == PG_translate == > PG_refcounts"), PG_refcounts and PG_translate always need to be set > together. > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts > are replaced by calls to paging_mode_translate. Would it be a good idea to merge all three and have only PG_external? > Signed-off-by: Wei Liu <wei.liu2@citrix.com> Acked-by: Tim Deegan <tim@xen.org> with one adjustment: > /* common paging mode bits */ > #define PG_mode_shift 10 > -/* Refcounts based on shadow tables instead of guest tables */ > -#define PG_refcounts (XEN_DOMCTL_SHADOW_ENABLE_REFCOUNT << PG_mode_shift) > /* Enable log dirty mode */ > #define PG_log_dirty (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift) > /* Xen does p2m translation, not guest */ Please add "and handles refcounts" to the comment describing PG_translate. Cheers, Tim.
On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote: > At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote: > > After 404595352 ("x86/paging: Enforce PG_external == PG_translate == > > PG_refcounts"), PG_refcounts and PG_translate always need to be set > > together. > > > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts > > are replaced by calls to paging_mode_translate. > > Would it be a good idea to merge all three and have only PG_external? > My reverse-engineering is that when PV guest is migrated it has PG_external and (the new) PG_translate. I would be happy to squash all three into one if you tell me I'm wrong. :-) > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > Acked-by: Tim Deegan <tim@xen.org> > > with one adjustment: > > > /* common paging mode bits */ > > #define PG_mode_shift 10 > > -/* Refcounts based on shadow tables instead of guest tables */ > > -#define PG_refcounts (XEN_DOMCTL_SHADOW_ENABLE_REFCOUNT << PG_mode_shift) > > /* Enable log dirty mode */ > > #define PG_log_dirty (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift) > > /* Xen does p2m translation, not guest */ > > Please add "and handles refcounts" to the comment describing PG_translate. > Sure.
On Thu, Aug 24, 2017 at 11:05:36AM +0100, Wei Liu wrote: > On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote: > > At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote: > > > After 404595352 ("x86/paging: Enforce PG_external == PG_translate == > > > PG_refcounts"), PG_refcounts and PG_translate always need to be set > > > together. > > > > > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts > > > are replaced by calls to paging_mode_translate. > > > > Would it be a good idea to merge all three and have only PG_external? > > > > My reverse-engineering is that when PV guest is migrated it has > PG_external and (the new) PG_translate. Sorry, I meant (!PG_external && PG_translate)
On 24/08/17 11:07, Wei Liu wrote: > On Thu, Aug 24, 2017 at 11:05:36AM +0100, Wei Liu wrote: >> On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote: >>> At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote: >>>> After 404595352 ("x86/paging: Enforce PG_external == PG_translate == >>>> PG_refcounts"), PG_refcounts and PG_translate always need to be set >>>> together. >>>> >>>> Squash PG_refcounts to simplify code. All calls paging_mode_refcounts >>>> are replaced by calls to paging_mode_translate. >>> Would it be a good idea to merge all three and have only PG_external? >>> >> My reverse-engineering is that when PV guest is migrated it has >> PG_external and (the new) PG_translate. > Sorry, I meant (!PG_external && PG_translate) PV guests migrating should get logdirty and none of PG_external|PG_translate|PG_refcounts ~Andrew
On Thu, Aug 24, 2017 at 11:09:38AM +0100, Andrew Cooper wrote: > On 24/08/17 11:07, Wei Liu wrote: > > On Thu, Aug 24, 2017 at 11:05:36AM +0100, Wei Liu wrote: > >> On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote: > >>> At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote: > >>>> After 404595352 ("x86/paging: Enforce PG_external == PG_translate == > >>>> PG_refcounts"), PG_refcounts and PG_translate always need to be set > >>>> together. > >>>> > >>>> Squash PG_refcounts to simplify code. All calls paging_mode_refcounts > >>>> are replaced by calls to paging_mode_translate. > >>> Would it be a good idea to merge all three and have only PG_external? > >>> > >> My reverse-engineering is that when PV guest is migrated it has > >> PG_external and (the new) PG_translate. > > Sorry, I meant (!PG_external && PG_translate) > > PV guests migrating should get logdirty and none of > PG_external|PG_translate|PG_refcounts > OK, in that case I will squash PG_external as well.
At 11:05 +0100 on 24 Aug (1503572736), Wei Liu wrote: > On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote: > > At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote: > > > After 404595352 ("x86/paging: Enforce PG_external == PG_translate == > > > PG_refcounts"), PG_refcounts and PG_translate always need to be set > > > together. > > > > > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts > > > are replaced by calls to paging_mode_translate. > > > > Would it be a good idea to merge all three and have only PG_external? > > > > My reverse-engineering is that when PV guest is migrated it has > PG_external and (the new) PG_translate. > > I would be happy to squash all three into one if you tell me I'm wrong. > :-) I _think_ you're wrong :) but can't check right now as I'm semi-offline. Migrating PV guests should have paging enabled but not any of external, translate or refcounts. There was once code that used PG_translate with PV guests but never AFAIK checked in to upstream Xen. There was some talk of using that combination for PVH at one point but IIRC PVH now uses translate|refcounts|external like HVM. Cheers, Tim.
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 9b4b9596d8..bbe545b165 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1017,8 +1017,6 @@ int arch_set_info_guest( if ( !cr3_page ) rc = -EINVAL; - else if ( paging_mode_refcounts(d) ) - /* nothing */; else if ( cr3_page == v->arch.old_guest_table ) { v->arch.old_guest_table = NULL; @@ -1040,7 +1038,7 @@ int arch_set_info_guest( break; case 0: if ( !compat && !VM_ASSIST(d, m2p_strict) && - !paging_mode_refcounts(d) ) + !paging_mode_translate(d) ) fill_ro_mpt(cr3_gfn); break; default: @@ -1061,7 +1059,7 @@ int arch_set_info_guest( if ( !cr3_page ) rc = -EINVAL; - else if ( !paging_mode_refcounts(d) ) + else if ( !paging_mode_translate(d) ) { rc = get_page_type_preemptible(cr3_page, PGT_root_page_table); switch ( rc ) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 6cb903def5..eee0ff422e 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -589,7 +589,7 @@ int hvm_domain_initialise(struct domain *d, unsigned long domcr_flags, hvm_init_cacheattr_region_list(d); - rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); + rc = paging_enable(d, PG_translate|PG_external); if ( rc != 0 ) goto fail0; diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index ed77270586..8adb7b6649 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1925,7 +1925,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e, if ( unlikely(__copy_from_user(&ol1e, pl1e, sizeof(ol1e)) != 0) ) return -EFAULT; - ASSERT(!paging_mode_refcounts(pt_dom)); + ASSERT(!paging_mode_translate(pt_dom)); if ( l1e_get_flags(nl1e) & _PAGE_PRESENT ) { @@ -2257,7 +2257,7 @@ int get_page(struct page_info *page, struct domain *domain) if ( likely(owner == domain) ) return 1; - if ( !paging_mode_refcounts(domain) && !domain->is_dying ) + if ( !paging_mode_translate(domain) && !domain->is_dying ) gprintk(XENLOG_INFO, "Error mfn %"PRI_mfn": rd=%d od=%d caf=%08lx taf=%" PRtype_info "\n", mfn_x(page_to_mfn(page)), domain->domain_id, @@ -2719,7 +2719,7 @@ int vcpu_destroy_pagetables(struct vcpu *v) if ( mfn ) { page = mfn_to_page(_mfn(mfn)); - if ( paging_mode_refcounts(v->domain) ) + if ( paging_mode_translate(v->domain) ) put_page(page); else rc = put_page_and_type_preemptible(page); @@ -2740,7 +2740,7 @@ int vcpu_destroy_pagetables(struct vcpu *v) if ( mfn ) { page = mfn_to_page(_mfn(mfn)); - if ( paging_mode_refcounts(v->domain) ) + if ( paging_mode_translate(v->domain) ) put_page(page); else rc = put_page_and_type_preemptible(page); @@ -2811,7 +2811,7 @@ int new_guest_cr3(unsigned long mfn) return 0; } - rc = paging_mode_refcounts(d) + rc = paging_mode_translate(d) ? (get_page_from_mfn(_mfn(mfn), d) ? 0 : -EINVAL) : get_page_and_type_from_mfn(_mfn(mfn), PGT_root_page_table, d, 0, 1); switch ( rc ) @@ -2829,7 +2829,7 @@ int new_guest_cr3(unsigned long mfn) invalidate_shadow_ldt(curr, 0); - if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) ) + if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_translate(d) ) fill_ro_mpt(mfn); curr->arch.guest_table = pagetable_from_pfn(mfn); update_cr3(curr); @@ -2840,7 +2840,7 @@ int new_guest_cr3(unsigned long mfn) { struct page_info *page = mfn_to_page(_mfn(old_base_mfn)); - if ( paging_mode_refcounts(d) ) + if ( paging_mode_translate(d) ) put_page(page); else switch ( rc = put_page_and_type_preemptible(page) ) @@ -3059,7 +3059,7 @@ long do_mmuext_op( if ( (op.cmd - MMUEXT_PIN_L1_TABLE) > (CONFIG_PAGING_LEVELS - 1) ) break; - if ( paging_mode_refcounts(pg_owner) ) + if ( paging_mode_translate(pg_owner) ) break; page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC); @@ -3121,7 +3121,7 @@ long do_mmuext_op( break; case MMUEXT_UNPIN_TABLE: - if ( paging_mode_refcounts(pg_owner) ) + if ( paging_mode_translate(pg_owner) ) break; page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC); @@ -3564,7 +3564,7 @@ long do_mmu_update( p2m_type_t p2mt; rc = -EOPNOTSUPP; - if ( unlikely(paging_mode_refcounts(pt_owner)) ) + if ( unlikely(paging_mode_translate(pt_owner)) ) break; xsm_needed |= XSM_MMU_NORMAL_UPDATE; diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c index 75f5fc0024..354d96d4cc 100644 --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -839,11 +839,11 @@ int paging_enable(struct domain *d, u32 mode) if ( mode & ~PG_MASK ) return -EINVAL; - /* All of external|translate|refcounts, or none. */ - switch ( mode & (PG_external | PG_translate | PG_refcounts) ) + /* Both of external|translate, or none. */ + switch ( mode & (PG_external | PG_translate) ) { case 0: - case PG_external | PG_translate | PG_refcounts: + case PG_external | PG_translate: break; default: return -EINVAL; @@ -881,8 +881,6 @@ void paging_dump_domain_info(struct domain *d) printk("shadow "); if ( paging_mode_hap(d) ) printk("hap "); - if ( paging_mode_refcounts(d) ) - printk("refcounts "); if ( paging_mode_log_dirty(d) ) printk("log_dirty "); if ( paging_mode_translate(d) ) diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h index 64bf2f968a..496a80a1ad 100644 --- a/xen/include/asm-x86/paging.h +++ b/xen/include/asm-x86/paging.h @@ -47,8 +47,6 @@ /* common paging mode bits */ #define PG_mode_shift 10 -/* Refcounts based on shadow tables instead of guest tables */ -#define PG_refcounts (XEN_DOMCTL_SHADOW_ENABLE_REFCOUNT << PG_mode_shift) /* Enable log dirty mode */ #define PG_log_dirty (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift) /* Xen does p2m translation, not guest */ @@ -58,13 +56,12 @@ #define PG_external (XEN_DOMCTL_SHADOW_ENABLE_EXTERNAL << PG_mode_shift) /* All paging modes. */ -#define PG_MASK (PG_refcounts | PG_log_dirty | PG_translate | PG_external) +#define PG_MASK (PG_log_dirty | PG_translate | PG_external) #define paging_mode_enabled(_d) (!!(_d)->arch.paging.mode) #define paging_mode_shadow(_d) (!!((_d)->arch.paging.mode & PG_SH_enable)) #define paging_mode_hap(_d) (!!((_d)->arch.paging.mode & PG_HAP_enable)) -#define paging_mode_refcounts(_d) (!!((_d)->arch.paging.mode & PG_refcounts)) #define paging_mode_log_dirty(_d) (!!((_d)->arch.paging.mode & PG_log_dirty)) #define paging_mode_translate(_d) (!!((_d)->arch.paging.mode & PG_translate)) #define paging_mode_external(_d) (!!((_d)->arch.paging.mode & PG_external)) diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h index 678b5d48bb..e0607b9620 100644 --- a/xen/include/asm-x86/shadow.h +++ b/xen/include/asm-x86/shadow.h @@ -35,7 +35,7 @@ #define shadow_mode_enabled(_d) paging_mode_shadow(_d) #define shadow_mode_refcounts(_d) (paging_mode_shadow(_d) && \ - paging_mode_refcounts(_d)) + paging_mode_translate(_d)) #define shadow_mode_log_dirty(_d) (paging_mode_shadow(_d) && \ paging_mode_log_dirty(_d)) #define shadow_mode_translate(_d) (paging_mode_shadow(_d) && \
After 404595352 ("x86/paging: Enforce PG_external == PG_translate == PG_refcounts"), PG_refcounts and PG_translate always need to be set together. Squash PG_refcounts to simplify code. All calls paging_mode_refcounts are replaced by calls to paging_mode_translate. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Tim Deegan <tim@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/domain.c | 6 ++---- xen/arch/x86/hvm/hvm.c | 2 +- xen/arch/x86/mm.c | 20 ++++++++++---------- xen/arch/x86/mm/paging.c | 8 +++----- xen/include/asm-x86/paging.h | 5 +---- xen/include/asm-x86/shadow.h | 2 +- 6 files changed, 18 insertions(+), 25 deletions(-)