| Message ID | 20180221140259.29360-13-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN | expand |
On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote: > A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to > the guest memory. > > Not functional change intended Is there a reason to not make all guest accessors tyep-safe instead? Wei.
On 23/02/18 17:26, Wei Liu wrote: > On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote: >> A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to >> the guest memory. >> >> Not functional change intended > > Is there a reason to not make all guest accessors tyep-safe instead? Could you clarify what you mean? MFN and xen_pfn_t have different type on Arm. So I am not sure how you would be able to make them typesafe. Cheers,
On Fri, Feb 23, 2018 at 05:46:39PM +0000, Julien Grall wrote: > > > On 23/02/18 17:26, Wei Liu wrote: > > On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote: > > > A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to > > > the guest memory. > > > > > > Not functional change intended > > > > Is there a reason to not make all guest accessors tyep-safe instead? > > Could you clarify what you mean? MFN and xen_pfn_t have different type on > Arm. So I am not sure how you would be able to make them typesafe. What I meant was to make copy_{to,from}_guest* type-safe. I just feel it a bit strange you only created a wrapper for this file. I wonder why. Note I'm just asking question. That's not necessarily a good idea to turn them all in the end. Wei.
Hi, On 23/02/18 18:05, Wei Liu wrote: > On Fri, Feb 23, 2018 at 05:46:39PM +0000, Julien Grall wrote: >> >> >> On 23/02/18 17:26, Wei Liu wrote: >>> On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote: >>>> A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to >>>> the guest memory. >>>> >>>> Not functional change intended >>> >>> Is there a reason to not make all guest accessors tyep-safe instead? >> >> Could you clarify what you mean? MFN and xen_pfn_t have different type on >> Arm. So I am not sure how you would be able to make them typesafe. > > What I meant was to make copy_{to,from}_guest* type-safe. I just feel it > a bit strange you only created a wrapper for this file. I wonder why. Oh, I can have a look at that. > > Note I'm just asking question. That's not necessarily a good idea to > turn them all in the end. Cheers,
On Fri, Feb 23, 2018 at 06:06:55PM +0000, Julien Grall wrote: > Hi, > > On 23/02/18 18:05, Wei Liu wrote: > > On Fri, Feb 23, 2018 at 05:46:39PM +0000, Julien Grall wrote: > > > > > > > > > On 23/02/18 17:26, Wei Liu wrote: > > > > On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote: > > > > > A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to > > > > > the guest memory. > > > > > > > > > > Not functional change intended > > > > > > > > Is there a reason to not make all guest accessors tyep-safe instead? > > > > > > Could you clarify what you mean? MFN and xen_pfn_t have different type on > > > Arm. So I am not sure how you would be able to make them typesafe. > > > > What I meant was to make copy_{to,from}_guest* type-safe. I just feel it > > a bit strange you only created a wrapper for this file. I wonder why. > > Oh, I can have a look at that. > Before you do any real work please wait for other people to comment. I haven't entirely convinced myself it is a good idea. ;-) Wei.
>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote: > @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d) > return min(order, MAX_ORDER + 0U); > } > > +/* Helper to copy a typesafe MFN to guest */ > +#define copy_mfn_to_guest(hnd, off, mfn) \ > + ({ \ > + xen_pfn_t mfn_ = mfn_x(mfn); \ > + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ > + }) Hmm, not really nice, but what do you do. > static void increase_reservation(struct memop_args *a) > { > struct page_info *page; > unsigned long i; > - xen_pfn_t mfn; > + mfn_t mfn; Please move this declaration ... > @@ -133,7 +146,7 @@ static void increase_reservation(struct memop_args *a) > !guest_handle_is_null(a->extent_list) ) > { > mfn = page_to_mfn(page); ... here, making the assignment its initializer. Or even avoid the local variable altogether, as the macro has already got one. Same elsewhere (whichever of the two variants fits), albeit maybe in the other cases the scope can't be shrunk much. Jan
Hi Jan, On 02/03/18 15:34, Jan Beulich wrote: >>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote: >> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d) >> return min(order, MAX_ORDER + 0U); >> } >> >> +/* Helper to copy a typesafe MFN to guest */ >> +#define copy_mfn_to_guest(hnd, off, mfn) \ >> + ({ \ >> + xen_pfn_t mfn_ = mfn_x(mfn); \ >> + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ >> + }) > > Hmm, not really nice, but what do you do. I am open to better suggestion. I wanted to avoid the conversion all over the code. Also, do you have an opinion on Wei's suggestion: "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it a bit strange you only created a wrapper for this file. I wonder why. Note I'm just asking question. That's not necessarily a good idea to turn them all in the end." > >> static void increase_reservation(struct memop_args *a) >> { >> struct page_info *page; >> unsigned long i; >> - xen_pfn_t mfn; >> + mfn_t mfn; > > Please move this declaration ... > >> @@ -133,7 +146,7 @@ static void increase_reservation(struct memop_args *a) >> !guest_handle_is_null(a->extent_list) ) >> { >> mfn = page_to_mfn(page); > > ... here, making the assignment its initializer. Or even avoid the > local variable altogether, as the macro has already got one. Same > elsewhere (whichever of the two variants fits), albeit maybe in the > other cases the scope can't be shrunk much. I will have a look. Cheers,
>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote: > On 02/03/18 15:34, Jan Beulich wrote: >>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote: >>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d) >>> return min(order, MAX_ORDER + 0U); >>> } >>> >>> +/* Helper to copy a typesafe MFN to guest */ >>> +#define copy_mfn_to_guest(hnd, off, mfn) \ >>> + ({ \ >>> + xen_pfn_t mfn_ = mfn_x(mfn); \ >>> + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ >>> + }) >> >> Hmm, not really nice, but what do you do. > > I am open to better suggestion. I wanted to avoid the conversion all > over the code. I have no better suggestion, I'm sorry, hence the "but what do you do." > Also, do you have an opinion on Wei's suggestion: > > "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it > a bit strange you only created a wrapper for this file. I wonder why. > > Note I'm just asking question. That's not necessarily a good idea to > turn them all in the end." Well, I didn't really understand what he's after (in the context of this series) - copy_{to,from}_guest() don't take or return MFNs or GFNs. Jan
On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote: > >>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote: > > On 02/03/18 15:34, Jan Beulich wrote: > >>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote: > >>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d) > >>> return min(order, MAX_ORDER + 0U); > >>> } > >>> > >>> +/* Helper to copy a typesafe MFN to guest */ > >>> +#define copy_mfn_to_guest(hnd, off, mfn) \ > >>> + ({ \ > >>> + xen_pfn_t mfn_ = mfn_x(mfn); \ > >>> + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ > >>> + }) > >> > >> Hmm, not really nice, but what do you do. > > > > I am open to better suggestion. I wanted to avoid the conversion all > > over the code. > > I have no better suggestion, I'm sorry, hence the "but what do > you do." > > > Also, do you have an opinion on Wei's suggestion: > > > > "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it > > a bit strange you only created a wrapper for this file. I wonder why. > > > > Note I'm just asking question. That's not necessarily a good idea to > > turn them all in the end." > > Well, I didn't really understand what he's after (in the context of > this series) - copy_{to,from}_guest() don't take or return MFNs or > GFNs. > Fundamentally Julien's patch is to wrap around an existing API for this one file only. Why is this file special? Why not just make that class of APIs do what he wants? But that is going to be intrusive and a bit counter-intuitive. (In the spirit of unblocking things, I won't insist making such change.) Wei.
Hi, On 03/09/2018 05:33 PM, Wei Liu wrote: > On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote: >>>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote: >>> On 02/03/18 15:34, Jan Beulich wrote: >>>>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote: >>>>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d) >>>>> return min(order, MAX_ORDER + 0U); >>>>> } >>>>> >>>>> +/* Helper to copy a typesafe MFN to guest */ >>>>> +#define copy_mfn_to_guest(hnd, off, mfn) \ >>>>> + ({ \ >>>>> + xen_pfn_t mfn_ = mfn_x(mfn); \ >>>>> + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ >>>>> + }) >>>> >>>> Hmm, not really nice, but what do you do. >>> >>> I am open to better suggestion. I wanted to avoid the conversion all >>> over the code. >> >> I have no better suggestion, I'm sorry, hence the "but what do >> you do." >> >>> Also, do you have an opinion on Wei's suggestion: >>> >>> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it >>> a bit strange you only created a wrapper for this file. I wonder why. >>> >>> Note I'm just asking question. That's not necessarily a good idea to >>> turn them all in the end." >> >> Well, I didn't really understand what he's after (in the context of >> this series) - copy_{to,from}_guest() don't take or return MFNs or >> GFNs. >> > > Fundamentally Julien's patch is to wrap around an existing API for this > one file only. Why is this file special? Why not just make that class of > APIs do what he wants? > > But that is going to be intrusive and a bit counter-intuitive. I have quickly looked at it. The major problem I can see is it is not possible to generically define for any typesafe. Indeed, TYPE_SAFE(...) cannot define new macro and, AFAICT, it is not feasible to define static inline for copy_* helpers. So we would need to introduce macros for each typesafe by hand. I can move copy_mfn_to_guest in xen/mm.h if people think it could be useful. Cheers,
>>> Julien Grall <julien.grall@arm.com> 03/11/18 8:44 PM >>> >On 03/09/2018 05:33 PM, Wei Liu wrote: >> On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote: >>>>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote: >>>> Also, do you have an opinion on Wei's suggestion: >>>> >>>> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it >>>> a bit strange you only created a wrapper for this file. I wonder why. >>>> >>>> Note I'm just asking question. That's not necessarily a good idea to >>>> turn them all in the end." >>> >>> Well, I didn't really understand what he's after (in the context of >>> this series) - copy_{to,from}_guest() don't take or return MFNs or >>> GFNs. >>> >> >> Fundamentally Julien's patch is to wrap around an existing API for this >> one file only. Why is this file special? Why not just make that class of >> APIs do what he wants? >> >> But that is going to be intrusive and a bit counter-intuitive. > >I have quickly looked at it. The major problem I can see is it is not >possible to generically define for any typesafe. Indeed, TYPE_SAFE(...) >cannot define new macro and, AFAICT, it is not feasible to define static >inline for copy_* helpers. > >So we would need to introduce macros for each typesafe by hand. I can >move copy_mfn_to_guest in xen/mm.h if people think it could be useful. First of all - how often do we copy in/out individual MFNs? Not in many places, I think. Hence I'm afraid I continue to not see the value of such a construct, especially not as a wider-than-file-scope one. Jan
Hi Jan, On 03/12/2018 06:39 AM, Jan Beulich wrote: >>>> Julien Grall <julien.grall@arm.com> 03/11/18 8:44 PM >>> >> On 03/09/2018 05:33 PM, Wei Liu wrote: >>> On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote: >>>>>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote: >>>>> Also, do you have an opinion on Wei's suggestion: >>>>> >>>>> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it >>>>> a bit strange you only created a wrapper for this file. I wonder why. >>>>> >>>>> Note I'm just asking question. That's not necessarily a good idea to >>>>> turn them all in the end." >>>> >>>> Well, I didn't really understand what he's after (in the context of >>>> this series) - copy_{to,from}_guest() don't take or return MFNs or >>>> GFNs. >>>> >>> >>> Fundamentally Julien's patch is to wrap around an existing API for this >>> one file only. Why is this file special? Why not just make that class of >>> APIs do what he wants? >>> >>> But that is going to be intrusive and a bit counter-intuitive. >> >> I have quickly looked at it. The major problem I can see is it is not >> possible to generically define for any typesafe. Indeed, TYPE_SAFE(...) >> cannot define new macro and, AFAICT, it is not feasible to define static >> inline for copy_* helpers. >> >> So we would need to introduce macros for each typesafe by hand. I can >> move copy_mfn_to_guest in xen/mm.h if people think it could be useful. > > First of all - how often do we copy in/out individual MFNs? Not in many places, > I think. Hence I'm afraid I continue to not see the value of such a construct, > especially not as a wider-than-file-scope one. I think you are right. Looking at the interface, we tend copy copy more often a GFN. We might want to provide an helper for typesafe GFN in the future. Cheers.
diff --git a/xen/common/memory.c b/xen/common/memory.c index 59d23a2a98..93d856df02 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -33,6 +33,12 @@ #include <asm/guest.h> #endif +/* Override macros from asm/page.h to make them work with mfn_t */ +#undef page_to_mfn +#define page_to_mfn(pg) _mfn(__page_to_mfn(pg)) +#undef mfn_to_page +#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn)) + struct memop_args { /* INPUT */ struct domain *domain; /* Domain to be affected. */ @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d) return min(order, MAX_ORDER + 0U); } +/* Helper to copy a typesafe MFN to guest */ +#define copy_mfn_to_guest(hnd, off, mfn) \ + ({ \ + xen_pfn_t mfn_ = mfn_x(mfn); \ + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ + }) + static void increase_reservation(struct memop_args *a) { struct page_info *page; unsigned long i; - xen_pfn_t mfn; + mfn_t mfn; struct domain *d = a->domain; if ( !guest_handle_is_null(a->extent_list) && @@ -133,7 +146,7 @@ static void increase_reservation(struct memop_args *a) !guest_handle_is_null(a->extent_list) ) { mfn = page_to_mfn(page); - if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) ) + if ( unlikely(copy_mfn_to_guest(a->extent_list, i, mfn)) ) goto out; } } @@ -146,7 +159,8 @@ static void populate_physmap(struct memop_args *a) { struct page_info *page; unsigned int i, j; - xen_pfn_t gpfn, mfn; + xen_pfn_t gpfn; + mfn_t mfn; struct domain *d = a->domain, *curr_d = current->domain; bool need_tlbflush = false; uint32_t tlbflush_timestamp = 0; @@ -205,14 +219,15 @@ static void populate_physmap(struct memop_args *a) { if ( is_domain_direct_mapped(d) ) { - mfn = gpfn; + mfn = _mfn(gpfn); - for ( j = 0; j < (1U << a->extent_order); j++, mfn++ ) + for ( j = 0; j < (1U << a->extent_order); j++, + mfn = mfn_add(mfn, 1) ) { - if ( !mfn_valid(_mfn(mfn)) ) + if ( !mfn_valid(mfn) ) { - gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_xen_pfn"\n", - mfn); + gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_mfn"\n", + mfn_x(mfn)); goto out; } @@ -220,14 +235,14 @@ static void populate_physmap(struct memop_args *a) if ( !get_page(page, d) ) { gdprintk(XENLOG_INFO, - "mfn %#"PRI_xen_pfn" doesn't belong to d%d\n", - mfn, d->domain_id); + "mfn %#"PRI_mfn" doesn't belong to d%d\n", + mfn_x(mfn), d->domain_id); goto out; } put_page(page); } - mfn = gpfn; + mfn = _mfn(gpfn); } else { @@ -253,15 +268,15 @@ static void populate_physmap(struct memop_args *a) mfn = page_to_mfn(page); } - guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order); + guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order); if ( !paging_mode_translate(d) ) { for ( j = 0; j < (1U << a->extent_order); j++ ) - set_gpfn_from_mfn(mfn + j, gpfn + j); + set_gpfn_from_mfn(mfn_x(mfn_add(mfn, j)), gpfn + j); /* Inform the domain of the new page's machine address. */ - if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) ) + if ( unlikely(copy_mfn_to_guest(a->extent_list, i, mfn)) ) goto out; } } @@ -304,7 +319,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) if ( p2mt == p2m_ram_paging_out ) { ASSERT(mfn_valid(mfn)); - page = mfn_to_page(mfn_x(mfn)); + page = mfn_to_page(mfn); if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) put_page(page); } @@ -349,7 +364,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) } #endif /* CONFIG_X86 */ - page = mfn_to_page(mfn_x(mfn)); + page = mfn_to_page(mfn); if ( unlikely(!get_page(page, d)) ) { put_gfn(d, gmfn); @@ -490,7 +505,8 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) PAGE_LIST_HEAD(in_chunk_list); PAGE_LIST_HEAD(out_chunk_list); unsigned long in_chunk_order, out_chunk_order; - xen_pfn_t gpfn, gmfn, mfn; + xen_pfn_t gpfn, gmfn; + mfn_t mfn; unsigned long i, j, k; unsigned int memflags = 0; long rc = 0; @@ -612,7 +628,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) p2m_type_t p2mt; /* Shared pages cannot be exchanged */ - mfn = mfn_x(get_gfn_unshare(d, gmfn + k, &p2mt)); + mfn = get_gfn_unshare(d, gmfn + k, &p2mt); if ( p2m_is_shared(p2mt) ) { put_gfn(d, gmfn + k); @@ -620,9 +636,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) goto fail; } #else /* !CONFIG_X86 */ - mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn + k))); + mfn = gfn_to_mfn(d, _gfn(gmfn + k)); #endif - if ( unlikely(!mfn_valid(_mfn(mfn))) ) + if ( unlikely(!mfn_valid(mfn)) ) { put_gfn(d, gmfn + k); rc = -EINVAL; @@ -669,10 +685,10 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) if ( !test_and_clear_bit(_PGC_allocated, &page->count_info) ) BUG(); mfn = page_to_mfn(page); - gfn = mfn_to_gmfn(d, mfn); + gfn = mfn_to_gmfn(d, mfn_x(mfn)); /* Pages were unshared above */ BUG_ON(SHARED_M2P(gfn)); - if ( guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), 0) ) + if ( guest_physmap_remove_page(d, _gfn(gfn), mfn, 0) ) domain_crash(d); put_page(page); } @@ -717,16 +733,16 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) } mfn = page_to_mfn(page); - guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), + guest_physmap_add_page(d, _gfn(gpfn), mfn, exch.out.extent_order); if ( !paging_mode_translate(d) ) { for ( k = 0; k < (1UL << exch.out.extent_order); k++ ) - set_gpfn_from_mfn(mfn + k, gpfn + k); - if ( __copy_to_guest_offset(exch.out.extent_start, - (i << out_chunk_order) + j, - &mfn, 1) ) + set_gpfn_from_mfn(mfn_x(mfn_add(mfn, k)), gpfn + k); + if ( copy_mfn_to_guest(exch.out.extent_start, + (i << out_chunk_order) + j, + mfn) ) rc = -EFAULT; } } @@ -1221,7 +1237,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( page ) { rc = guest_physmap_remove_page(d, _gfn(xrfp.gpfn), - _mfn(page_to_mfn(page)), 0); + page_to_mfn(page), 0); put_page(page); } else
A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to the guest memory. Not functional change intended Signed-off-by: Julien Grall <julien.grall@arm.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: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Changes in v4: - Patch added --- xen/common/memory.c | 72 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 28 deletions(-)