Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
Hi Jan,
On 03/12/2018 06:39 AM, Jan Beulich wrote:
Julien Grall 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, 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.
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
>>> Julien Grall 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, 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
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, wrote:
On 02/03/18 15:34, Jan Beulich wrote:
On 21.02.18 at 15:02, 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote:
> >>> On 05.03.18 at 15:18, wrote:
> > On 02/03/18 15:34, Jan Beulich wrote:
> > On 21.02.18 at 15:02, 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.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
>>> On 05.03.18 at 15:18, wrote:
> On 02/03/18 15:34, Jan Beulich wrote:
> On 21.02.18 at 15:02, 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
Hi Jan,
On 02/03/18 15:34, Jan Beulich wrote:
On 21.02.18 at 15:02, 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,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
>>> On 21.02.18 at 15:02, 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
On Fri, Feb 23, 2018 at 06:06:55PM +, Julien Grall wrote:
> Hi,
>
> On 23/02/18 18:05, Wei Liu wrote:
> > On Fri, Feb 23, 2018 at 05:46:39PM +, Julien Grall wrote:
> > >
> > >
> > > On 23/02/18 17:26, Wei Liu wrote:
> > > > On Wed, Feb 21, 2018 at 02:02:55PM +, 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.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
Hi,
On 23/02/18 18:05, Wei Liu wrote:
On Fri, Feb 23, 2018 at 05:46:39PM +, Julien Grall wrote:
On 23/02/18 17:26, Wei Liu wrote:
On Wed, Feb 21, 2018 at 02:02:55PM +, 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,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
On Fri, Feb 23, 2018 at 05:46:39PM +, Julien Grall wrote:
>
>
> On 23/02/18 17:26, Wei Liu wrote:
> > On Wed, Feb 21, 2018 at 02:02:55PM +, 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.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
On 23/02/18 17:26, Wei Liu wrote: On Wed, Feb 21, 2018 at 02:02:55PM +, 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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 12/16] xen/mm: Switch common/memory.c to use typesafe MFN
On Wed, Feb 21, 2018 at 02:02:55PM +, 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. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
