[Xen-devel,v2] xen: grant-table: Simplify get_paged_frame

Message ID 20170918162752.14091-1-julien.grall@arm.com
State New
Headers show
Series
  • [Xen-devel,v2] xen: grant-table: Simplify get_paged_frame
Related show

Commit Message

Julien Grall Sept. 18, 2017, 4:27 p.m.
The implementation of get_paged_frame is currently different whether the
architecture support sharing memory or paging memory. Both
version are extremely similar so it is possible to consolidate in a
single implementation.

The main difference is the x86 version will allow grant on foreign page
when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
are only allowed for PVH Dom0. It seems that foreign pages should never
be granted so deny them

The check for shared/paged memory are now gated with the respective ifdef.
Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
Arm.

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 v2:
        - Deny grant on foreign page (aligned with the ARM code)
        - Use #ifdef rather than #if defined
        - Update commit message
        - Fix typo in the title

get_page_from_gfn will be able to get reference on foreign page and as
per my understanding will allow to grant page on foreign memory.

This was not allowed with a simple get_page(...) on the ARM
implementation (no sharing nor paging supprot) but is allowed on the x86
implementation due to get_page_from_gfn.

On x86, foreign pages are currently only allowed for PVH dom0, so I
think it is not a big deal for now.

On Arm, foreign pages can be present on any domain. So this patch would
permit grant on foreing pages.

This patch will deny granting foreign pages. Jan Beulich is happy with
it. Any other opinions?
---
 xen/common/grant_table.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

Comments

Roger Pau Monné Sept. 18, 2017, 4:58 p.m. | #1
On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
> The implementation of get_paged_frame is currently different whether the
> architecture support sharing memory or paging memory. Both
> version are extremely similar so it is possible to consolidate in a
> single implementation.
> 
> The main difference is the x86 version will allow grant on foreign page
> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
> are only allowed for PVH Dom0. It seems that foreign pages should never
> be granted so deny them
> 
> The check for shared/paged memory are now gated with the respective ifdef.
> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
> Arm.
> 
> 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 v2:
>         - Deny grant on foreign page (aligned with the ARM code)
>         - Use #ifdef rather than #if defined
>         - Update commit message
>         - Fix typo in the title
> 
> get_page_from_gfn will be able to get reference on foreign page and as
> per my understanding will allow to grant page on foreign memory.
> 
> This was not allowed with a simple get_page(...) on the ARM
> implementation (no sharing nor paging supprot) but is allowed on the x86
> implementation due to get_page_from_gfn.
> 
> On x86, foreign pages are currently only allowed for PVH dom0, so I
> think it is not a big deal for now.
> 
> On Arm, foreign pages can be present on any domain. So this patch would
> permit grant on foreing pages.
> 
> This patch will deny granting foreign pages. Jan Beulich is happy with
> it. Any other opinions?

Won't this break QEMU running in stub domains?

I haven't tested it, but I'm afraid QEMU running in a stub domain
might try to grant a foreign frame. Ie: the emulated network code in
QEMU might try to grant a foreign frame in order to forward operations
from emulated devices to PV frontends.

Roger.
Julien Grall Sept. 18, 2017, 5:32 p.m. | #2
Hi Roger,

On 18/09/17 17:58, Roger Pau Monné wrote:
> On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
>> The implementation of get_paged_frame is currently different whether the
>> architecture support sharing memory or paging memory. Both
>> version are extremely similar so it is possible to consolidate in a
>> single implementation.
>>
>> The main difference is the x86 version will allow grant on foreign page
>> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
>> are only allowed for PVH Dom0. It seems that foreign pages should never
>> be granted so deny them
>>
>> The check for shared/paged memory are now gated with the respective ifdef.
>> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
>> Arm.
>>
>> 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 v2:
>>          - Deny grant on foreign page (aligned with the ARM code)
>>          - Use #ifdef rather than #if defined
>>          - Update commit message
>>          - Fix typo in the title
>>
>> get_page_from_gfn will be able to get reference on foreign page and as
>> per my understanding will allow to grant page on foreign memory.
>>
>> This was not allowed with a simple get_page(...) on the ARM
>> implementation (no sharing nor paging supprot) but is allowed on the x86
>> implementation due to get_page_from_gfn.
>>
>> On x86, foreign pages are currently only allowed for PVH dom0, so I
>> think it is not a big deal for now.
>>
>> On Arm, foreign pages can be present on any domain. So this patch would
>> permit grant on foreing pages.
>>
>> This patch will deny granting foreign pages. Jan Beulich is happy with
>> it. Any other opinions?
> 
> Won't this break QEMU running in stub domains?
> 
> I haven't tested it, but I'm afraid QEMU running in a stub domain
> might try to grant a foreign frame. Ie: the emulated network code in
> QEMU might try to grant a foreign frame in order to forward operations
> from emulated devices to PV frontends.

I don't think it will break any existing setup because foreign mapping 
are only allowed for the hardware domain (see p2m_add_foreign).

Now, what should be expected in the future? I am not sure why we would 
allow grant on foreign mapping. It is very similar to QEMU granting a 
page that was granted by another domain, this is not allowed today.

Cheers,
Roger Pau Monné Sept. 19, 2017, 7:13 a.m. | #3
On Mon, Sep 18, 2017 at 06:32:22PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 18/09/17 17:58, Roger Pau Monné wrote:
> > On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
> > > The implementation of get_paged_frame is currently different whether the
> > > architecture support sharing memory or paging memory. Both
> > > version are extremely similar so it is possible to consolidate in a
> > > single implementation.
> > > 
> > > The main difference is the x86 version will allow grant on foreign page
> > > when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
> > > are only allowed for PVH Dom0. It seems that foreign pages should never
> > > be granted so deny them
> > > 
> > > The check for shared/paged memory are now gated with the respective ifdef.
> > > Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
> > > Arm.
> > > 
> > > 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 v2:
> > >          - Deny grant on foreign page (aligned with the ARM code)
> > >          - Use #ifdef rather than #if defined
> > >          - Update commit message
> > >          - Fix typo in the title
> > > 
> > > get_page_from_gfn will be able to get reference on foreign page and as
> > > per my understanding will allow to grant page on foreign memory.
> > > 
> > > This was not allowed with a simple get_page(...) on the ARM
> > > implementation (no sharing nor paging supprot) but is allowed on the x86
> > > implementation due to get_page_from_gfn.
> > > 
> > > On x86, foreign pages are currently only allowed for PVH dom0, so I
> > > think it is not a big deal for now.
> > > 
> > > On Arm, foreign pages can be present on any domain. So this patch would
> > > permit grant on foreing pages.
> > > 
> > > This patch will deny granting foreign pages. Jan Beulich is happy with
> > > it. Any other opinions?
> > 
> > Won't this break QEMU running in stub domains?
> > 
> > I haven't tested it, but I'm afraid QEMU running in a stub domain
> > might try to grant a foreign frame. Ie: the emulated network code in
> > QEMU might try to grant a foreign frame in order to forward operations
> > from emulated devices to PV frontends.
> 
> I don't think it will break any existing setup because foreign mapping are
> only allowed for the hardware domain (see p2m_add_foreign).

IIRC this only applies to auto-translated (HVM) domains, not to PV
domains. QEMU stubdomains are PV guests.

Roger.
Paul Durrant Sept. 19, 2017, 8:34 a.m. | #4
> -----Original Message-----

> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of

> Julien Grall

> Sent: 18 September 2017 17:28

> To: xen-devel@lists.xen.org

> Cc: sstabellini@kernel.org; Wei Liu <wei.liu2@citrix.com>; George Dunlap

> <George.Dunlap@citrix.com>; Andrew Cooper

> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim

> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>;

> jbeulich@suse.com

> Subject: [Xen-devel] [PATCH v2] xen: grant-table: Simplify get_paged_frame

> 

> The implementation of get_paged_frame is currently different whether the

> architecture support sharing memory or paging memory. Both

> version are extremely similar so it is possible to consolidate in a

> single implementation.

> 

> The main difference is the x86 version will allow grant on foreign page

> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign

> pages

> are only allowed for PVH Dom0. It seems that foreign pages should never

> be granted so deny them

> 

> The check for shared/paged memory are now gated with the respective

> ifdef.

> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented

> for

> Arm.

> 

> 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 v2:

>         - Deny grant on foreign page (aligned with the ARM code)

>         - Use #ifdef rather than #if defined

>         - Update commit message

>         - Fix typo in the title

> 

> get_page_from_gfn will be able to get reference on foreign page and as

> per my understanding will allow to grant page on foreign memory.

> 

> This was not allowed with a simple get_page(...) on the ARM

> implementation (no sharing nor paging supprot) but is allowed on the x86

> implementation due to get_page_from_gfn.

> 

> On x86, foreign pages are currently only allowed for PVH dom0, so I

> think it is not a big deal for now.

> 

> On Arm, foreign pages can be present on any domain. So this patch would

> permit grant on foreing pages.

> 

> This patch will deny granting foreign pages. Jan Beulich is happy with

> it. Any other opinions?


That seems reasonable to me. You can't grant frames that have been grant mapped from another domain (except by using v2 transitive grants) so disallowing granting of priv mapped frames is at least consistent.

I do wonder whether this function belongs in the grant table code though. Getting the page from a (d, gfn) tuple is probably something that's needed in a few places and hence putting the code in common/memory.c (with suitable adjustment to the error values) would seem more appropriate.

  Paul

> ---

>  xen/common/grant_table.c | 19 ++++++++-----------

>  1 file changed, 8 insertions(+), 11 deletions(-)

> 

> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c

> index c3895e6201..a6a168df6e 100644

> --- a/xen/common/grant_table.c

> +++ b/xen/common/grant_table.c

> @@ -259,7 +259,6 @@ static int get_paged_frame(unsigned long gfn,

> unsigned long *frame,

>                             struct domain *rd)

>  {

>      int rc = GNTST_okay;

> -#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)

>      p2m_type_t p2mt;

> 

>      *page = get_page_from_gfn(rd, gfn, &p2mt,

> @@ -267,26 +266,24 @@ static int get_paged_frame(unsigned long gfn,

> unsigned long *frame,

>      if ( !(*page) )

>      {

>          *frame = mfn_x(INVALID_MFN);

> +#ifdef P2M_SHARED_TYPES

>          if ( p2m_is_shared(p2mt) )

>              return GNTST_eagain;

> +#endif

> +#ifdef P2M_PAGES_TYPES

>          if ( p2m_is_paging(p2mt) )

>          {

>              p2m_mem_paging_populate(rd, gfn);

>              return GNTST_eagain;

>          }

> +#endif

>          return GNTST_bad_page;

>      }

> +

> +    if ( p2m_is_foreign(p2mt) )

> +        return GNTST_bad_page;

> +

>      *frame = page_to_mfn(*page);

> -#else

> -    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));

> -    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;

> -    if ( (!(*page)) || (!get_page(*page, rd)) )

> -    {

> -        *frame = mfn_x(INVALID_MFN);

> -        *page = NULL;

> -        rc = GNTST_bad_page;

> -    }

> -#endif

> 

>      return rc;

>  }

> --

> 2.11.0

> 

> 

> _______________________________________________

> Xen-devel mailing list

> Xen-devel@lists.xen.org

> https://lists.xen.org/xen-devel
Jan Beulich Sept. 19, 2017, 8:56 a.m. | #5
>>> On 19.09.17 at 10:34, <Paul.Durrant@citrix.com> wrote:
> I do wonder whether this function belongs in the grant table code though. 
> Getting the page from a (d, gfn) tuple is probably something that's needed in 
> a few places and hence putting the code in common/memory.c (with suitable 
> adjustment to the error values) would seem more appropriate.

That's been true from the very beginning of the existence of
the function, I think.

Jan
Julien Grall Sept. 19, 2017, 9:39 a.m. | #6
Hi,

On 19/09/17 09:56, Jan Beulich wrote:
>>>> On 19.09.17 at 10:34, <Paul.Durrant@citrix.com> wrote:
>> I do wonder whether this function belongs in the grant table code though.
>> Getting the page from a (d, gfn) tuple is probably something that's needed in
>> a few places and hence putting the code in common/memory.c (with suitable
>> adjustment to the error values) would seem more appropriate.
> 
> That's been true from the very beginning of the existence of
> the function, I think.

I am not sure how this function would fit in common/memory.c code. We 
already have get_page_from_gfn to get a page from the tuple (d, gfn).

This function adds more check that may not fit everyone. The only place 
I could see potential usage is prepare_ring_for_helper. But what would 
be a suitable name given?

Cheers,
Julien Grall Sept. 19, 2017, 9:44 a.m. | #7
Hi,

On 19/09/17 08:13, Roger Pau Monné wrote:
> On Mon, Sep 18, 2017 at 06:32:22PM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 18/09/17 17:58, Roger Pau Monné wrote:
>>> On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
>>>> The implementation of get_paged_frame is currently different whether the
>>>> architecture support sharing memory or paging memory. Both
>>>> version are extremely similar so it is possible to consolidate in a
>>>> single implementation.
>>>>
>>>> The main difference is the x86 version will allow grant on foreign page
>>>> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
>>>> are only allowed for PVH Dom0. It seems that foreign pages should never
>>>> be granted so deny them
>>>>
>>>> The check for shared/paged memory are now gated with the respective ifdef.
>>>> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
>>>> Arm.
>>>>
>>>> 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 v2:
>>>>           - Deny grant on foreign page (aligned with the ARM code)
>>>>           - Use #ifdef rather than #if defined
>>>>           - Update commit message
>>>>           - Fix typo in the title
>>>>
>>>> get_page_from_gfn will be able to get reference on foreign page and as
>>>> per my understanding will allow to grant page on foreign memory.
>>>>
>>>> This was not allowed with a simple get_page(...) on the ARM
>>>> implementation (no sharing nor paging supprot) but is allowed on the x86
>>>> implementation due to get_page_from_gfn.
>>>>
>>>> On x86, foreign pages are currently only allowed for PVH dom0, so I
>>>> think it is not a big deal for now.
>>>>
>>>> On Arm, foreign pages can be present on any domain. So this patch would
>>>> permit grant on foreing pages.
>>>>
>>>> This patch will deny granting foreign pages. Jan Beulich is happy with
>>>> it. Any other opinions?
>>>
>>> Won't this break QEMU running in stub domains?
>>>
>>> I haven't tested it, but I'm afraid QEMU running in a stub domain
>>> might try to grant a foreign frame. Ie: the emulated network code in
>>> QEMU might try to grant a foreign frame in order to forward operations
>>> from emulated devices to PV frontends.
>>
>> I don't think it will break any existing setup because foreign mapping are
>> only allowed for the hardware domain (see p2m_add_foreign).
> 
> IIRC this only applies to auto-translated (HVM) domains, not to PV
> domains. QEMU stubdomains are PV guests.

Still, p2m_map_foreign can only be used for auto-translated domains.

Furthermore, if you look at the implementation of get_page_from_gfn, for 
PV domain is basically a get_page(page, d) that can't not work on 
foreign pages.

So I really don't see any problem here.

Cheers,
Paul Durrant Sept. 19, 2017, 9:47 a.m. | #8
> -----Original Message-----

> From: Julien Grall [mailto:julien.grall@arm.com]

> Sent: 19 September 2017 10:40

> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant

> <Paul.Durrant@citrix.com>

> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap

> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu

> <wei.liu2@citrix.com>; sstabellini@kernel.org; xen-devel@lists.xen.org; Tim

> (Xen.org) <tim@xen.org>

> Subject: Re: [Xen-devel] [PATCH v2] xen: grant-table: Simplify

> get_paged_frame

> 

> Hi,

> 

> On 19/09/17 09:56, Jan Beulich wrote:

> >>>> On 19.09.17 at 10:34, <Paul.Durrant@citrix.com> wrote:

> >> I do wonder whether this function belongs in the grant table code though.

> >> Getting the page from a (d, gfn) tuple is probably something that's

> needed in

> >> a few places and hence putting the code in common/memory.c (with

> suitable

> >> adjustment to the error values) would seem more appropriate.

> >

> > That's been true from the very beginning of the existence of

> > the function, I think.

> 

> I am not sure how this function would fit in common/memory.c code. We

> already have get_page_from_gfn to get a page from the tuple (d, gfn).

> 


But that doesn't have the extra logic for populating and unsharing right? Otherwise why would get_paged_frame() need to exist at all?

> This function adds more check that may not fit everyone. The only place

> I could see potential usage is prepare_ring_for_helper. But what would

> be a suitable name given?

> 


I would leave the name alone and just move the code.

  Paul

> Cheers,

> 

> --

> Julien Grall
Jan Beulich Sept. 19, 2017, 9:57 a.m. | #9
>>> On 19.09.17 at 11:39, <julien.grall@arm.com> wrote:
> On 19/09/17 09:56, Jan Beulich wrote:
>>>>> On 19.09.17 at 10:34, <Paul.Durrant@citrix.com> wrote:
>>> I do wonder whether this function belongs in the grant table code though.
>>> Getting the page from a (d, gfn) tuple is probably something that's needed in
>>> a few places and hence putting the code in common/memory.c (with suitable
>>> adjustment to the error values) would seem more appropriate.
>> 
>> That's been true from the very beginning of the existence of
>> the function, I think.
> 
> I am not sure how this function would fit in common/memory.c code. We 
> already have get_page_from_gfn to get a page from the tuple (d, gfn).
> 
> This function adds more check that may not fit everyone. The only place 
> I could see potential usage is prepare_ring_for_helper. But what would 
> be a suitable name given?

Actually I didn't mean to suggest you do anything beyond what
the patch already does. If you, Paul, or anyone else feel like
moving the function, it would probably better be a separate
patch (and we may then want to think of not only a better name,
but also where else such that function may be usable).

Jan
Julien Grall Sept. 19, 2017, 10:01 a.m. | #10
On 19/09/17 10:47, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 19 September 2017 10:40
>> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
>> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
>> <wei.liu2@citrix.com>; sstabellini@kernel.org; xen-devel@lists.xen.org; Tim
>> (Xen.org) <tim@xen.org>
>> Subject: Re: [Xen-devel] [PATCH v2] xen: grant-table: Simplify
>> get_paged_frame
>>
>> Hi,
>>
>> On 19/09/17 09:56, Jan Beulich wrote:
>>>>>> On 19.09.17 at 10:34, <Paul.Durrant@citrix.com> wrote:
>>>> I do wonder whether this function belongs in the grant table code though.
>>>> Getting the page from a (d, gfn) tuple is probably something that's
>> needed in
>>>> a few places and hence putting the code in common/memory.c (with
>> suitable
>>>> adjustment to the error values) would seem more appropriate.
>>>
>>> That's been true from the very beginning of the existence of
>>> the function, I think.
>>
>> I am not sure how this function would fit in common/memory.c code. We
>> already have get_page_from_gfn to get a page from the tuple (d, gfn).
>>
> 
> But that doesn't have the extra logic for populating and unsharing right? Otherwise why would get_paged_frame() need to exist at all?

This code does not have the logic to unshare. It just denies it.

There are multiple places in grant-table which require similar check. 
Hence why the helper is here to avoid duplicating the code in grant_table.c.

With this patch, this will contain specific check for grant mapping, 
such as denying foreign mapping. Are we going to impose that for 
get_paged_frame in common/memory.c?

> 
>> This function adds more check that may not fit everyone. The only place
>> I could see potential usage is prepare_ring_for_helper. But what would
>> be a suitable name given?
>>
> 
> I would leave the name alone and just move the code.
The name does not make sense in common/memory.c. Why would you allow 
populating but not unsharing? This looks to me a decision that fits 
grant-table and not necessarily the rest of the Xen.

So I still don't think this would be a suitable function in common/memory.c

Cheers,
Paul Durrant Sept. 19, 2017, 10:05 a.m. | #11
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 19 September 2017 10:58
> To: Julien Grall <julien.grall@arm.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> sstabellini@kernel.org; xen-devel@lists.xen.org; Tim (Xen.org)
> <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v2] xen: grant-table: Simplify
> get_paged_frame
> 
> >>> On 19.09.17 at 11:39, <julien.grall@arm.com> wrote:
> > On 19/09/17 09:56, Jan Beulich wrote:
> >>>>> On 19.09.17 at 10:34, <Paul.Durrant@citrix.com> wrote:
> >>> I do wonder whether this function belongs in the grant table code
> though.
> >>> Getting the page from a (d, gfn) tuple is probably something that's
> needed in
> >>> a few places and hence putting the code in common/memory.c (with
> suitable
> >>> adjustment to the error values) would seem more appropriate.
> >>
> >> That's been true from the very beginning of the existence of
> >> the function, I think.
> >
> > I am not sure how this function would fit in common/memory.c code. We
> > already have get_page_from_gfn to get a page from the tuple (d, gfn).
> >
> > This function adds more check that may not fit everyone. The only place
> > I could see potential usage is prepare_ring_for_helper. But what would
> > be a suitable name given?
> 
> Actually I didn't mean to suggest you do anything beyond what
> the patch already does. If you, Paul, or anyone else feel like
> moving the function, it would probably better be a separate
> patch (and we may then want to think of not only a better name,
> but also where else such that function may be usable).
> 

Ok, I'll do it then. I need the same functionality for PV-IOMMU.

  Paul

> Jan
Wei Liu Sept. 19, 2017, 10:38 a.m. | #12
On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
> The implementation of get_paged_frame is currently different whether the
> architecture support sharing memory or paging memory. Both
> version are extremely similar so it is possible to consolidate in a
> single implementation.
> 
> The main difference is the x86 version will allow grant on foreign page
> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
> are only allowed for PVH Dom0. It seems that foreign pages should never
> be granted so deny them
> 
> The check for shared/paged memory are now gated with the respective ifdef.
> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
> Arm.
> 
> 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 v2:
>         - Deny grant on foreign page (aligned with the ARM code)
>         - Use #ifdef rather than #if defined
>         - Update commit message
>         - Fix typo in the title
> 
> get_page_from_gfn will be able to get reference on foreign page and as
> per my understanding will allow to grant page on foreign memory.
> 
> This was not allowed with a simple get_page(...) on the ARM
> implementation (no sharing nor paging supprot) but is allowed on the x86
> implementation due to get_page_from_gfn.
> 
> On x86, foreign pages are currently only allowed for PVH dom0, so I
> think it is not a big deal for now.
> 
> On Arm, foreign pages can be present on any domain. So this patch would
> permit grant on foreing pages.
> 
> This patch will deny granting foreign pages. Jan Beulich is happy with
> it. Any other opinions?
> ---
>  xen/common/grant_table.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index c3895e6201..a6a168df6e 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -259,7 +259,6 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>                             struct domain *rd)
>  {
>      int rc = GNTST_okay;
> -#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
>      p2m_type_t p2mt;
>  
>      *page = get_page_from_gfn(rd, gfn, &p2mt,

While you're at it, mind dropping the pointless brackets around
readonly?

> @@ -267,26 +266,24 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>      if ( !(*page) )

And here as well.

>      {
>          *frame = mfn_x(INVALID_MFN);
> +#ifdef P2M_SHARED_TYPES
>          if ( p2m_is_shared(p2mt) )
>              return GNTST_eagain;
> +#endif
> +#ifdef P2M_PAGES_TYPES
>          if ( p2m_is_paging(p2mt) )
>          {
>              p2m_mem_paging_populate(rd, gfn);
>              return GNTST_eagain;
>          }
> +#endif
>          return GNTST_bad_page;
>      }
> +
> +    if ( p2m_is_foreign(p2mt) )
> +        return GNTST_bad_page;

You only get here when you have taken a ref on the page. You should drop
the ref before returning.
Julien Grall Sept. 19, 2017, 10:43 a.m. | #13
Hi Wei,

On 19/09/17 11:38, Wei Liu wrote:
> On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
>> The implementation of get_paged_frame is currently different whether the
>> architecture support sharing memory or paging memory. Both
>> version are extremely similar so it is possible to consolidate in a
>> single implementation.
>>
>> The main difference is the x86 version will allow grant on foreign page
>> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
>> are only allowed for PVH Dom0. It seems that foreign pages should never
>> be granted so deny them
>>
>> The check for shared/paged memory are now gated with the respective ifdef.
>> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
>> Arm.
>>
>> 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 v2:
>>          - Deny grant on foreign page (aligned with the ARM code)
>>          - Use #ifdef rather than #if defined
>>          - Update commit message
>>          - Fix typo in the title
>>
>> get_page_from_gfn will be able to get reference on foreign page and as
>> per my understanding will allow to grant page on foreign memory.
>>
>> This was not allowed with a simple get_page(...) on the ARM
>> implementation (no sharing nor paging supprot) but is allowed on the x86
>> implementation due to get_page_from_gfn.
>>
>> On x86, foreign pages are currently only allowed for PVH dom0, so I
>> think it is not a big deal for now.
>>
>> On Arm, foreign pages can be present on any domain. So this patch would
>> permit grant on foreing pages.
>>
>> This patch will deny granting foreign pages. Jan Beulich is happy with
>> it. Any other opinions?
>> ---
>>   xen/common/grant_table.c | 19 ++++++++-----------
>>   1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index c3895e6201..a6a168df6e 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -259,7 +259,6 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>>                              struct domain *rd)
>>   {
>>       int rc = GNTST_okay;
>> -#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
>>       p2m_type_t p2mt;
>>   
>>       *page = get_page_from_gfn(rd, gfn, &p2mt,
> 
> While you're at it, mind dropping the pointless brackets around
> readonly?

Will do.

>> @@ -267,26 +266,24 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>>       if ( !(*page) )
> 
> And here as well.
> 
>>       {
>>           *frame = mfn_x(INVALID_MFN);
>> +#ifdef P2M_SHARED_TYPES
>>           if ( p2m_is_shared(p2mt) )
>>               return GNTST_eagain;
>> +#endif
>> +#ifdef P2M_PAGES_TYPES
>>           if ( p2m_is_paging(p2mt) )
>>           {
>>               p2m_mem_paging_populate(rd, gfn);
>>               return GNTST_eagain;
>>           }
>> +#endif
>>           return GNTST_bad_page;
>>       }
>> +
>> +    if ( p2m_is_foreign(p2mt) )
>> +        return GNTST_bad_page;
> 
> You only get here when you have taken a ref on the page. You should drop
> the ref before returning.

Hmmm yes. I will send a new version with that fixed.

Cheers,
Roger Pau Monné Sept. 19, 2017, 11:40 a.m. | #14
On Tue, Sep 19, 2017 at 10:44:53AM +0100, Julien Grall wrote:
> Hi,
> 
> On 19/09/17 08:13, Roger Pau Monné wrote:
> > On Mon, Sep 18, 2017 at 06:32:22PM +0100, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 18/09/17 17:58, Roger Pau Monné wrote:
> > > > On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
> > > > > The implementation of get_paged_frame is currently different whether the
> > > > > architecture support sharing memory or paging memory. Both
> > > > > version are extremely similar so it is possible to consolidate in a
> > > > > single implementation.
> > > > > 
> > > > > The main difference is the x86 version will allow grant on foreign page
> > > > > when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
> > > > > are only allowed for PVH Dom0. It seems that foreign pages should never
> > > > > be granted so deny them
> > > > > 
> > > > > The check for shared/paged memory are now gated with the respective ifdef.
> > > > > Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
> > > > > Arm.
> > > > > 
> > > > > 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 v2:
> > > > >           - Deny grant on foreign page (aligned with the ARM code)
> > > > >           - Use #ifdef rather than #if defined
> > > > >           - Update commit message
> > > > >           - Fix typo in the title
> > > > > 
> > > > > get_page_from_gfn will be able to get reference on foreign page and as
> > > > > per my understanding will allow to grant page on foreign memory.
> > > > > 
> > > > > This was not allowed with a simple get_page(...) on the ARM
> > > > > implementation (no sharing nor paging supprot) but is allowed on the x86
> > > > > implementation due to get_page_from_gfn.
> > > > > 
> > > > > On x86, foreign pages are currently only allowed for PVH dom0, so I
> > > > > think it is not a big deal for now.
> > > > > 
> > > > > On Arm, foreign pages can be present on any domain. So this patch would
> > > > > permit grant on foreing pages.
> > > > > 
> > > > > This patch will deny granting foreign pages. Jan Beulich is happy with
> > > > > it. Any other opinions?
> > > > 
> > > > Won't this break QEMU running in stub domains?
> > > > 
> > > > I haven't tested it, but I'm afraid QEMU running in a stub domain
> > > > might try to grant a foreign frame. Ie: the emulated network code in
> > > > QEMU might try to grant a foreign frame in order to forward operations
> > > > from emulated devices to PV frontends.
> > > 
> > > I don't think it will break any existing setup because foreign mapping are
> > > only allowed for the hardware domain (see p2m_add_foreign).
> > 
> > IIRC this only applies to auto-translated (HVM) domains, not to PV
> > domains. QEMU stubdomains are PV guests.
> 
> Still, p2m_map_foreign can only be used for auto-translated domains.

Oh, so you are limiting this only for auto-translated guests? Should
have looked more closely at the commit message.

Roger.
Julien Grall Sept. 20, 2017, 11:28 a.m. | #15
Hi Roger,

On 19/09/17 12:40, Roger Pau Monné wrote:
> On Tue, Sep 19, 2017 at 10:44:53AM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 19/09/17 08:13, Roger Pau Monné wrote:
>>> On Mon, Sep 18, 2017 at 06:32:22PM +0100, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 18/09/17 17:58, Roger Pau Monné wrote:
>>>>> On Mon, Sep 18, 2017 at 05:27:52PM +0100, Julien Grall wrote:
>>>>>> The implementation of get_paged_frame is currently different whether the
>>>>>> architecture support sharing memory or paging memory. Both
>>>>>> version are extremely similar so it is possible to consolidate in a
>>>>>> single implementation.
>>>>>>
>>>>>> The main difference is the x86 version will allow grant on foreign page
>>>>>> when using HVM/PVH whilst Arm does not. At the moment, on x86 foreign pages
>>>>>> are only allowed for PVH Dom0. It seems that foreign pages should never
>>>>>> be granted so deny them
>>>>>>
>>>>>> The check for shared/paged memory are now gated with the respective ifdef.
>>>>>> Potentially, dummy p2m_is_shared/p2m_is_paging could be implemented for
>>>>>> Arm.
>>>>>>
>>>>>> 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 v2:
>>>>>>            - Deny grant on foreign page (aligned with the ARM code)
>>>>>>            - Use #ifdef rather than #if defined
>>>>>>            - Update commit message
>>>>>>            - Fix typo in the title
>>>>>>
>>>>>> get_page_from_gfn will be able to get reference on foreign page and as
>>>>>> per my understanding will allow to grant page on foreign memory.
>>>>>>
>>>>>> This was not allowed with a simple get_page(...) on the ARM
>>>>>> implementation (no sharing nor paging supprot) but is allowed on the x86
>>>>>> implementation due to get_page_from_gfn.
>>>>>>
>>>>>> On x86, foreign pages are currently only allowed for PVH dom0, so I
>>>>>> think it is not a big deal for now.
>>>>>>
>>>>>> On Arm, foreign pages can be present on any domain. So this patch would
>>>>>> permit grant on foreing pages.
>>>>>>
>>>>>> This patch will deny granting foreign pages. Jan Beulich is happy with
>>>>>> it. Any other opinions?
>>>>>
>>>>> Won't this break QEMU running in stub domains?
>>>>>
>>>>> I haven't tested it, but I'm afraid QEMU running in a stub domain
>>>>> might try to grant a foreign frame. Ie: the emulated network code in
>>>>> QEMU might try to grant a foreign frame in order to forward operations
>>>>> from emulated devices to PV frontends.
>>>>
>>>> I don't think it will break any existing setup because foreign mapping are
>>>> only allowed for the hardware domain (see p2m_add_foreign).
>>>
>>> IIRC this only applies to auto-translated (HVM) domains, not to PV
>>> domains. QEMU stubdomains are PV guests.
>>
>> Still, p2m_map_foreign can only be used for auto-translated domains.
> 
> Oh, so you are limiting this only for auto-translated guests? Should
> have looked more closely at the commit message.

That's right. I am happy to update the commit message if it is not clear 
enough.

Cheers,

Patch

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c3895e6201..a6a168df6e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -259,7 +259,6 @@  static int get_paged_frame(unsigned long gfn, unsigned long *frame,
                            struct domain *rd)
 {
     int rc = GNTST_okay;
-#if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
     p2m_type_t p2mt;
 
     *page = get_page_from_gfn(rd, gfn, &p2mt,
@@ -267,26 +266,24 @@  static int get_paged_frame(unsigned long gfn, unsigned long *frame,
     if ( !(*page) )
     {
         *frame = mfn_x(INVALID_MFN);
+#ifdef P2M_SHARED_TYPES
         if ( p2m_is_shared(p2mt) )
             return GNTST_eagain;
+#endif
+#ifdef P2M_PAGES_TYPES
         if ( p2m_is_paging(p2mt) )
         {
             p2m_mem_paging_populate(rd, gfn);
             return GNTST_eagain;
         }
+#endif
         return GNTST_bad_page;
     }
+
+    if ( p2m_is_foreign(p2mt) )
+        return GNTST_bad_page;
+
     *frame = page_to_mfn(*page);
-#else
-    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
-    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;
-    if ( (!(*page)) || (!get_page(*page, rd)) )
-    {
-        *frame = mfn_x(INVALID_MFN);
-        *page = NULL;
-        rc = GNTST_bad_page;
-    }
-#endif
 
     return rc;
 }