| Message ID | 1466515243-27264-5-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Stefano, On 23/06/16 11:20, Stefano Stabellini wrote: > On Tue, 21 Jun 2016, Julien Grall wrote: >> The x86 version of the function xenmem_add_to_physmap_one contains >> variable name gpfn and gfn which make the code very confusing. >> I have left unchanged for now. >> >> Also, rename gpfn to gfn in the ARM version as the latter is the correct >> acronym for a guest physical frame. >> >> Finally, remove the trailing whitespace around the changes. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Acked-by: Jan Beulich <jbeulich@suse.com> >> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Tim Deegan <tim@xen.org> >> Cc: Wei Liu <wei.liu2@citrix.com> >> >> Changes in v3: >> - Add Jan's acked-by for non-ARM bits >> --- >> xen/arch/arm/mm.c | 10 +++++----- >> xen/arch/x86/mm.c | 15 +++++++-------- >> xen/common/memory.c | 6 +++--- >> xen/include/xen/mm.h | 2 +- >> 4 files changed, 16 insertions(+), 17 deletions(-) >> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index 5ab9b75..6882d54 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one( >> unsigned int space, >> union xen_add_to_physmap_batch_extra extra, >> unsigned long idx, >> - xen_pfn_t gpfn) >> + gfn_t gfn) > > I think there is a possible loss of information here: xen_pfn_t is > always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with > its current definition. Or am I missing something? > > In fact, given that ARM supports LPAE, shouldn't gfn by defined as > xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ? With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded on 28-bit. So unsigned long is perfectly fine here. Note that the truncation already occurs in subsequent caller because we are using unsigned long for the P2M parameters. > > >> { >> unsigned long mfn = 0; >> int rc; >> @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one( >> else >> return -EINVAL; >> } >> - >> - d->arch.grant_table_gpfn[idx] = gpfn; >> + >> + d->arch.grant_table_gpfn[idx] = gfn_x(gfn); > > Similarly grant_table_gpfn is an array of xen_pfn_t (uint64_t), while > gfn_x unboxes to the storage size of gfn (unsigned long). Patch #5 will switch grant_table_gpfn to an array of gfn_t. Regards,
On 23/06/16 14:06, Stefano Stabellini wrote: > On Thu, 23 Jun 2016, Julien Grall wrote: >> On 23/06/16 11:20, Stefano Stabellini wrote: >>> On Tue, 21 Jun 2016, Julien Grall wrote: >>> I think there is a possible loss of information here: xen_pfn_t is >>> always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with >>> its current definition. Or am I missing something? >>> >>> In fact, given that ARM supports LPAE, shouldn't gfn by defined as >>> xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ? >> >> With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded on >> 28-bit. So unsigned long is perfectly fine here. > > Somehow I have always assumed that the 40-bit limitation was just > temporary. That ARM in the future will just increase that number up to > 64-bit eventually. > > If you don't think there is any risk of that happening, then I am fine > with this. We just have to keep in mind that many of the guest > interfaces use xen_pfn_t, which has a different size on ARM. Currently, Aarch32 supports up to 40-bit whilst Aarch64 supports up to 48-bit (even 52-bit with ARMv8.2). So this should be ok for now. However, pretty much everywhere in Xen we assume that the frame number is unsigned long (see mm.c, p2m.c,...). We would have much more work to do than this small patch. I would rather start to switch to gfn/mfn internally and keep the underlying type as "unsigned long" until we effectively need 64-bit frame. The main reason is 64-bit frame will result into a bigger binary for ARM32 with no apparent reason (40-bit is hardcoded in setup_virt_paging). Switching to gfn/mfn will allow us to uint64_t where it will be required. Regards,
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 5ab9b75..6882d54 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one( unsigned int space, union xen_add_to_physmap_batch_extra extra, unsigned long idx, - xen_pfn_t gpfn) + gfn_t gfn) { unsigned long mfn = 0; int rc; @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one( else return -EINVAL; } - - d->arch.grant_table_gpfn[idx] = gpfn; + + d->arch.grant_table_gpfn[idx] = gfn_x(gfn); t = p2m_ram_rw; @@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one( if ( extra.res0 ) return -EOPNOTSUPP; - rc = map_dev_mmio_region(d, gpfn, 1, idx); + rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx); return rc; default: @@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one( } /* Map at new location. */ - rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t); + rc = guest_physmap_add_entry(d, gfn, _mfn(mfn), 0, t); /* If we fail to add the mapping, we need to drop the reference we * took earlier on foreign pages */ diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 7fbc94e..dbcf6cb 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4775,7 +4775,7 @@ int xenmem_add_to_physmap_one( unsigned int space, union xen_add_to_physmap_batch_extra extra, unsigned long idx, - xen_pfn_t gpfn) + gfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ @@ -4834,7 +4834,7 @@ int xenmem_add_to_physmap_one( break; } case XENMAPSPACE_gmfn_foreign: - return p2m_add_foreign(d, idx, gpfn, extra.foreign_domid); + return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid); default: break; } @@ -4849,19 +4849,18 @@ int xenmem_add_to_physmap_one( } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn), - PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, gpfn); + guest_remove_page(d, gfn_x(gpfn)); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, gpfn); + put_gfn(d, gfn_x(gpfn)); /* Unmap from old location, if any. */ old_gpfn = get_gpfn_from_mfn(mfn); @@ -4872,7 +4871,7 @@ int xenmem_add_to_physmap_one( guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) diff --git a/xen/common/memory.c b/xen/common/memory.c index a8a75e0..812334b 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -649,7 +649,7 @@ static int xenmem_add_to_physmap(struct domain *d, if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, extra, - xatp->idx, xatp->gpfn); + xatp->idx, _gfn(xatp->gpfn)); if ( xatp->size < start ) return -EILSEQ; @@ -666,7 +666,7 @@ static int xenmem_add_to_physmap(struct domain *d, while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, extra, - xatp->idx, xatp->gpfn); + xatp->idx, _gfn(xatp->gpfn)); if ( rc < 0 ) break; @@ -735,7 +735,7 @@ static int xenmem_add_to_physmap_batch(struct domain *d, rc = xenmem_add_to_physmap_one(d, xatpb->space, xatpb->u, - idx, gpfn); + idx, _gfn(gpfn)); if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) ) { diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index b62f473..afbb1a1 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -548,7 +548,7 @@ void scrub_one_page(struct page_info *); int xenmem_add_to_physmap_one(struct domain *d, unsigned int space, union xen_add_to_physmap_batch_extra extra, - unsigned long idx, xen_pfn_t gpfn); + unsigned long idx, gfn_t gfn); /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */