IOREQ server on Arm

IOREQ server on Arm

[Julien Grall]

Hi Paul,

I am looking at porting the IOREQ server infrastructure on Arm. I didn't 
need much modification to make it run for Arm. Although, the 
implementation could be simplified over the x86 implementation.

I noticed some issue while trying to implement the hypercall 
XENMEM_acquire_resource. Per my understanding, all the page mapped via 
that hypercall will use the type p2m_mapping_foreign.

This will result to trigger the ASSERT(fdom != dom) in get_page_from_gfn 
(asm-arm/p2m.h) because the IOREQ page has been allocated to the 
emulator domain and mapped to it. AFAICT x86 has the same assert in 
p2m_get_page_from_gfn(...).

IHMO, the ASSERT makes sense because you are only meant to map page 
belonging to other domain with that type.

So I am wondering whether IOREQ server running in PVH Dom0 has been 
tested? What would be the best course of action to fix the issue?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Jan Beulich]

>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> Hi Paul,
> 
> I am looking at porting the IOREQ server infrastructure on Arm. I didn't 
> need much modification to make it run for Arm. Although, the 
> implementation could be simplified over the x86 implementation.
> 
> I noticed some issue while trying to implement the hypercall 
> XENMEM_acquire_resource. Per my understanding, all the page mapped via 
> that hypercall will use the type p2m_mapping_foreign.
> 
> This will result to trigger the ASSERT(fdom != dom) in get_page_from_gfn 
> (asm-arm/p2m.h) because the IOREQ page has been allocated to the 
> emulator domain and mapped to it. AFAICT x86 has the same assert in 
> p2m_get_page_from_gfn(...).
> 
> IHMO, the ASSERT makes sense because you are only meant to map page 
> belonging to other domain with that type.
> 
> So I am wondering whether IOREQ server running in PVH Dom0 has been 
> tested? What would be the best course of action to fix the issue?

I think the p2m type needs to be chosen based on
XENMEM_rsrc_acq_caller_owned.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 26 September 2018 09:09
> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> >>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> > Hi Paul,
> >
> > I am looking at porting the IOREQ server infrastructure on Arm. I didn't
> > need much modification to make it run for Arm. Although, the
> > implementation could be simplified over the x86 implementation.
> >
> > I noticed some issue while trying to implement the hypercall
> > XENMEM_acquire_resource. Per my understanding, all the page mapped via
> > that hypercall will use the type p2m_mapping_foreign.
> >
> > This will result to trigger the ASSERT(fdom != dom) in get_page_from_gfn
> > (asm-arm/p2m.h) because the IOREQ page has been allocated to the
> > emulator domain and mapped to it. AFAICT x86 has the same assert in
> > p2m_get_page_from_gfn(...).
> >
> > IHMO, the ASSERT makes sense because you are only meant to map page
> > belonging to other domain with that type.
> >
> > So I am wondering whether IOREQ server running in PVH Dom0 has been
> > tested? What would be the best course of action to fix the issue?
> 
> I think the p2m type needs to be chosen based on
> XENMEM_rsrc_acq_caller_owned.
> 

Yes, that's correct. There is a FIXME clause in acquire_resource so that that only caller owned resources can be mapped by HVM/PVH domains. Thus the new call can be used for IOREQ server pages, but not grant frames.

  Paul

> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi,

On 09/26/2018 10:14 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 26 September 2018 09:09
>> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel <xen-devel@lists.xenproject.org>
>> Subject: Re: IOREQ server on Arm
>>
>>>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
>>> Hi Paul,
>>>
>>> I am looking at porting the IOREQ server infrastructure on Arm. I didn't
>>> need much modification to make it run for Arm. Although, the
>>> implementation could be simplified over the x86 implementation.
>>>
>>> I noticed some issue while trying to implement the hypercall
>>> XENMEM_acquire_resource. Per my understanding, all the page mapped via
>>> that hypercall will use the type p2m_mapping_foreign.
>>>
>>> This will result to trigger the ASSERT(fdom != dom) in get_page_from_gfn
>>> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
>>> emulator domain and mapped to it. AFAICT x86 has the same assert in
>>> p2m_get_page_from_gfn(...).
>>>
>>> IHMO, the ASSERT makes sense because you are only meant to map page
>>> belonging to other domain with that type.
>>>
>>> So I am wondering whether IOREQ server running in PVH Dom0 has been
>>> tested? What would be the best course of action to fix the issue?
>>
>> I think the p2m type needs to be chosen based on
>> XENMEM_rsrc_acq_caller_owned.
>>
> 
> Yes, that's correct. There is a FIXME clause in acquire_resource so that that only caller owned resources can be mapped by HVM/PVH domains. Thus the new call can be used for IOREQ server pages, but not grant frames.

I don't understand your last sentence. IOREQ is caller owned and 
therefore should work.

As I said, I don't have any problem with mapping the resource. The 
problem is when unmapping it because the region is set with 
p2m_mapping_foreign. You will reach the ASSERT(fdom != p2m->domain) in 
p2m_get_page_from_gfn.

Regardless the reference problem (we support it on Arm). Can you explain 
how this is working on PVH Dom0 today?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Andrew Cooper]

On 26/09/18 11:32, Julien Grall wrote:
> Hi,
>
> On 09/26/2018 10:14 AM, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 26 September 2018 09:09
>>> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
>>> <Paul.Durrant@citrix.com>
>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
>>> xen-
>>> devel <xen-devel@lists.xenproject.org>
>>> Subject: Re: IOREQ server on Arm
>>>
>>>>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
>>>> Hi Paul,
>>>>
>>>> I am looking at porting the IOREQ server infrastructure on Arm. I
>>>> didn't
>>>> need much modification to make it run for Arm. Although, the
>>>> implementation could be simplified over the x86 implementation.
>>>>
>>>> I noticed some issue while trying to implement the hypercall
>>>> XENMEM_acquire_resource. Per my understanding, all the page mapped via
>>>> that hypercall will use the type p2m_mapping_foreign.
>>>>
>>>> This will result to trigger the ASSERT(fdom != dom) in
>>>> get_page_from_gfn
>>>> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
>>>> emulator domain and mapped to it. AFAICT x86 has the same assert in
>>>> p2m_get_page_from_gfn(...).
>>>>
>>>> IHMO, the ASSERT makes sense because you are only meant to map page
>>>> belonging to other domain with that type.
>>>>
>>>> So I am wondering whether IOREQ server running in PVH Dom0 has been
>>>> tested? What would be the best course of action to fix the issue?
>>>
>>> I think the p2m type needs to be chosen based on
>>> XENMEM_rsrc_acq_caller_owned.
>>>
>>
>> Yes, that's correct. There is a FIXME clause in acquire_resource so
>> that that only caller owned resources can be mapped by HVM/PVH
>> domains. Thus the new call can be used for IOREQ server pages, but
>> not grant frames.
>
> I don't understand your last sentence. IOREQ is caller owned and
> therefore should work.
>
> As I said, I don't have any problem with mapping the resource. The
> problem is when unmapping it because the region is set with
> p2m_mapping_foreign. You will reach the ASSERT(fdom != p2m->domain) in
> p2m_get_page_from_gfn.
>
> Regardless the reference problem (we support it on Arm). Can you
> explain how this is working on PVH Dom0 today?

I doubt anyone has tried using it with a PVH Dom0.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi Jan,

On 09/26/2018 09:08 AM, Jan Beulich wrote:
>>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
>> Hi Paul,
>>
>> I am looking at porting the IOREQ server infrastructure on Arm. I didn't
>> need much modification to make it run for Arm. Although, the
>> implementation could be simplified over the x86 implementation.
>>
>> I noticed some issue while trying to implement the hypercall
>> XENMEM_acquire_resource. Per my understanding, all the page mapped via
>> that hypercall will use the type p2m_mapping_foreign.
>>
>> This will result to trigger the ASSERT(fdom != dom) in get_page_from_gfn
>> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
>> emulator domain and mapped to it. AFAICT x86 has the same assert in
>> p2m_get_page_from_gfn(...).
>>
>> IHMO, the ASSERT makes sense because you are only meant to map page
>> belonging to other domain with that type.
>>
>> So I am wondering whether IOREQ server running in PVH Dom0 has been
>> tested? What would be the best course of action to fix the issue?
> 
> I think the p2m type needs to be chosen based on
> XENMEM_rsrc_acq_caller_owned.

I am thinking to introduce p2m_mapping_owned. Or do we have a p2m_type 
that we could re-use?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 26 September 2018 11:33
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> Hi,
> 
> On 09/26/2018 10:14 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 26 September 2018 09:09
> >> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
> >> <Paul.Durrant@citrix.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-
> >> devel <xen-devel@lists.xenproject.org>
> >> Subject: Re: IOREQ server on Arm
> >>
> >>>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> >>> Hi Paul,
> >>>
> >>> I am looking at porting the IOREQ server infrastructure on Arm. I
> didn't
> >>> need much modification to make it run for Arm. Although, the
> >>> implementation could be simplified over the x86 implementation.
> >>>
> >>> I noticed some issue while trying to implement the hypercall
> >>> XENMEM_acquire_resource. Per my understanding, all the page mapped via
> >>> that hypercall will use the type p2m_mapping_foreign.
> >>>
> >>> This will result to trigger the ASSERT(fdom != dom) in
> get_page_from_gfn
> >>> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
> >>> emulator domain and mapped to it. AFAICT x86 has the same assert in
> >>> p2m_get_page_from_gfn(...).
> >>>
> >>> IHMO, the ASSERT makes sense because you are only meant to map page
> >>> belonging to other domain with that type.
> >>>
> >>> So I am wondering whether IOREQ server running in PVH Dom0 has been
> >>> tested? What would be the best course of action to fix the issue?
> >>
> >> I think the p2m type needs to be chosen based on
> >> XENMEM_rsrc_acq_caller_owned.
> >>
> >
> > Yes, that's correct. There is a FIXME clause in acquire_resource so that
> that only caller owned resources can be mapped by HVM/PVH domains. Thus
> the new call can be used for IOREQ server pages, but not grant frames.
> 
> I don't understand your last sentence. IOREQ is caller owned and
> therefore should work.

Yes, I was just saying that it is currently the only resource that should work.

> 
> As I said, I don't have any problem with mapping the resource. The
> problem is when unmapping it because the region is set with
> p2m_mapping_foreign. You will reach the ASSERT(fdom != p2m->domain) in
> p2m_get_page_from_gfn.
> 
> Regardless the reference problem (we support it on Arm). Can you explain
> how this is working on PVH Dom0 today?
> 

I never tested with a PVH dom0, but I did run tests with an HVM domU (and a Xen with the privilege checks hacked out). I guess I didn't re-test with HVM after the switch to caller-owned (which came quite late in review) and so missed this problem. Sorry about that.

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 26 September 2018 11:41
> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> Hi Jan,
> 
> On 09/26/2018 09:08 AM, Jan Beulich wrote:
> >>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> >> Hi Paul,
> >>
> >> I am looking at porting the IOREQ server infrastructure on Arm. I
> didn't
> >> need much modification to make it run for Arm. Although, the
> >> implementation could be simplified over the x86 implementation.
> >>
> >> I noticed some issue while trying to implement the hypercall
> >> XENMEM_acquire_resource. Per my understanding, all the page mapped via
> >> that hypercall will use the type p2m_mapping_foreign.
> >>
> >> This will result to trigger the ASSERT(fdom != dom) in
> get_page_from_gfn
> >> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
> >> emulator domain and mapped to it. AFAICT x86 has the same assert in
> >> p2m_get_page_from_gfn(...).
> >>
> >> IHMO, the ASSERT makes sense because you are only meant to map page
> >> belonging to other domain with that type.
> >>
> >> So I am wondering whether IOREQ server running in PVH Dom0 has been
> >> tested? What would be the best course of action to fix the issue?
> >
> > I think the p2m type needs to be chosen based on
> > XENMEM_rsrc_acq_caller_owned.
> 
> I am thinking to introduce p2m_mapping_owned. Or do we have a p2m_type
> that we could re-use?
> 

I think we should be able to just use p2m_ram_rw if it is caller owned.

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Jan Beulich]

>>> On 26.09.18 at 12:51, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 26 September 2018 11:41
>> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel <xen-devel@lists.xenproject.org>
>> Subject: Re: IOREQ server on Arm
>> 
>> Hi Jan,
>> 
>> On 09/26/2018 09:08 AM, Jan Beulich wrote:
>> >>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
>> >> Hi Paul,
>> >>
>> >> I am looking at porting the IOREQ server infrastructure on Arm. I
>> didn't
>> >> need much modification to make it run for Arm. Although, the
>> >> implementation could be simplified over the x86 implementation.
>> >>
>> >> I noticed some issue while trying to implement the hypercall
>> >> XENMEM_acquire_resource. Per my understanding, all the page mapped via
>> >> that hypercall will use the type p2m_mapping_foreign.
>> >>
>> >> This will result to trigger the ASSERT(fdom != dom) in
>> get_page_from_gfn
>> >> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
>> >> emulator domain and mapped to it. AFAICT x86 has the same assert in
>> >> p2m_get_page_from_gfn(...).
>> >>
>> >> IHMO, the ASSERT makes sense because you are only meant to map page
>> >> belonging to other domain with that type.
>> >>
>> >> So I am wondering whether IOREQ server running in PVH Dom0 has been
>> >> tested? What would be the best course of action to fix the issue?
>> >
>> > I think the p2m type needs to be chosen based on
>> > XENMEM_rsrc_acq_caller_owned.
>> 
>> I am thinking to introduce p2m_mapping_owned. Or do we have a p2m_type
>> that we could re-use?
>> 
> 
> I think we should be able to just use p2m_ram_rw if it is caller owned.

Yes, that's what I too would have thought. If there ever was a resource
which may only be mapped r/o, p2m_ram_ro should then be equally usable.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi Paul,

On 09/26/2018 11:51 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 26 September 2018 11:41
>> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel <xen-devel@lists.xenproject.org>
>> Subject: Re: IOREQ server on Arm
>>
>> Hi Jan,
>>
>> On 09/26/2018 09:08 AM, Jan Beulich wrote:_
>>>>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
>>>> Hi Paul,
>>>>
>>>> I am looking at porting the IOREQ server infrastructure on Arm. I
>> didn't
>>>> need much modification to make it run for Arm. Although, the
>>>> implementation could be simplified over the x86 implementation.
>>>>
>>>> I noticed some issue while trying to implement the hypercall
>>>> XENMEM_acquire_resource. Per my understanding, all the page mapped via
>>>> that hypercall will use the type p2m_mapping_foreign.
>>>>
>>>> This will result to trigger the ASSERT(fdom != dom) in
>> get_page_from_gfn
>>>> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
>>>> emulator domain and mapped to it. AFAICT x86 has the same assert in
>>>> p2m_get_page_from_gfn(...).
>>>>
>>>> IHMO, the ASSERT makes sense because you are only meant to map page
>>>> belonging to other domain with that type.
>>>>
>>>> So I am wondering whether IOREQ server running in PVH Dom0 has been
>>>> tested? What would be the best course of action to fix the issue?
>>>
>>> I think the p2m type needs to be chosen based on
>>> XENMEM_rsrc_acq_caller_owned.
>>
>> I am thinking to introduce p2m_mapping_owned. Or do we have a p2m_type
>> that we could re-use?
>>
> 
> I think we should be able to just use p2m_ram_rw if it is caller owned.

I thought about p2m_ram_rw but discarded because of the security 
implications. At least on Arm, this type can be used for foreign 
mapping, guest_copy helpers. This is not the case for p2m_mapping_foreign.

Do we want to allow thoses resources to be used in hypercall buffer 
and/or mapped by other guest via the foreign API? If not, then we want 
to use a different type.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 26 September 2018 11:58
> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: RE: IOREQ server on Arm
> 
> >>> On 26.09.18 at 12:51, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@arm.com]
> >> Sent: 26 September 2018 11:41
> >> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
> >> <Paul.Durrant@citrix.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-
> >> devel <xen-devel@lists.xenproject.org>
> >> Subject: Re: IOREQ server on Arm
> >>
> >> Hi Jan,
> >>
> >> On 09/26/2018 09:08 AM, Jan Beulich wrote:
> >> >>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> >> >> Hi Paul,
> >> >>
> >> >> I am looking at porting the IOREQ server infrastructure on Arm. I
> >> didn't
> >> >> need much modification to make it run for Arm. Although, the
> >> >> implementation could be simplified over the x86 implementation.
> >> >>
> >> >> I noticed some issue while trying to implement the hypercall
> >> >> XENMEM_acquire_resource. Per my understanding, all the page mapped
> via
> >> >> that hypercall will use the type p2m_mapping_foreign.
> >> >>
> >> >> This will result to trigger the ASSERT(fdom != dom) in
> >> get_page_from_gfn
> >> >> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
> >> >> emulator domain and mapped to it. AFAICT x86 has the same assert in
> >> >> p2m_get_page_from_gfn(...).
> >> >>
> >> >> IHMO, the ASSERT makes sense because you are only meant to map page
> >> >> belonging to other domain with that type.
> >> >>
> >> >> So I am wondering whether IOREQ server running in PVH Dom0 has been
> >> >> tested? What would be the best course of action to fix the issue?
> >> >
> >> > I think the p2m type needs to be chosen based on
> >> > XENMEM_rsrc_acq_caller_owned.
> >>
> >> I am thinking to introduce p2m_mapping_owned. Or do we have a p2m_type
> >> that we could re-use?
> >>
> >
> > I think we should be able to just use p2m_ram_rw if it is caller owned.
> 
> Yes, that's what I too would have thought. If there ever was a resource
> which may only be mapped r/o, p2m_ram_ro should then be equally usable.
> 

Yes, that's true. The only existent resources are read-write though so I guess another flag passed back to the caller could be used to indicate a read-only resource.

I was thinking along the lines of a patch like this:

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 996f94b103..82c18fa9ad 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1105,8 +1105,11 @@ static int acquire_resource(

         for ( i = 0; !rc && i < xmar.nr_frames; i++ )
         {
-            rc = set_foreign_p2m_entry(currd, gfn_list[i],
-                                       _mfn(mfn_list[i]));
+            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
+                guest_physmap_add_entry(currd, gfn_list[i],
+                                        _mfn(mfn_list[i]), 0, p2m_ram_rw) :
+                set_foreign_p2m_entry(currd, gfn_list[i],
+                                      _mfn(mfn_list[i]));
             /* rc should be -EIO for any iteration other than the first */
             if ( rc && i )
                 rc = -EIO;

But the guest_physmap_add_entry() is problematic as it will IOMMU map pages as well, which is probably not wanted.

  Paul 

> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 26 September 2018 12:01
> To: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich
> <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> Hi Paul,
> 
> On 09/26/2018 11:51 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@arm.com]
> >> Sent: 26 September 2018 11:41
> >> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
> >> <Paul.Durrant@citrix.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-
> >> devel <xen-devel@lists.xenproject.org>
> >> Subject: Re: IOREQ server on Arm
> >>
> >> Hi Jan,
> >>
> >> On 09/26/2018 09:08 AM, Jan Beulich wrote:_
> >>>>>> On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> >>>> Hi Paul,
> >>>>
> >>>> I am looking at porting the IOREQ server infrastructure on Arm. I
> >> didn't
> >>>> need much modification to make it run for Arm. Although, the
> >>>> implementation could be simplified over the x86 implementation.
> >>>>
> >>>> I noticed some issue while trying to implement the hypercall
> >>>> XENMEM_acquire_resource. Per my understanding, all the page mapped
> via
> >>>> that hypercall will use the type p2m_mapping_foreign.
> >>>>
> >>>> This will result to trigger the ASSERT(fdom != dom) in
> >> get_page_from_gfn
> >>>> (asm-arm/p2m.h) because the IOREQ page has been allocated to the
> >>>> emulator domain and mapped to it. AFAICT x86 has the same assert in
> >>>> p2m_get_page_from_gfn(...).
> >>>>
> >>>> IHMO, the ASSERT makes sense because you are only meant to map page
> >>>> belonging to other domain with that type.
> >>>>
> >>>> So I am wondering whether IOREQ server running in PVH Dom0 has been
> >>>> tested? What would be the best course of action to fix the issue?
> >>>
> >>> I think the p2m type needs to be chosen based on
> >>> XENMEM_rsrc_acq_caller_owned.
> >>
> >> I am thinking to introduce p2m_mapping_owned. Or do we have a p2m_type
> >> that we could re-use?
> >>
> >
> > I think we should be able to just use p2m_ram_rw if it is caller owned.
> 
> I thought about p2m_ram_rw but discarded because of the security
> implications. At least on Arm, this type can be used for foreign
> mapping, guest_copy helpers. This is not the case for p2m_mapping_foreign.
> 

Not sure. The emulator has to have privilege over the target, so any domain having privilege over the emulator would have privilege over the target, wouldn't it? So I don't think there is any security issue.

> Do we want to allow thoses resources to be used in hypercall buffer
> and/or mapped by other guest via the foreign API? If not, then we want
> to use a different type.
> 

I can't think of a use-case where we would want that, but I'm not sure there is any particular problem allowing it. Same goes for the IOMMU mappings that I mentioned to Jan though... not really desirable but not necessarily a problem (for existing resource types). A new p2m type may well be a better option.

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Jan Beulich]

>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1105,8 +1105,11 @@ static int acquire_resource(
> 
>          for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>          {
> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> -                                       _mfn(mfn_list[i]));
> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
> +                guest_physmap_add_entry(currd, gfn_list[i],
> +                                        _mfn(mfn_list[i]), 0, p2m_ram_rw) :
> +                set_foreign_p2m_entry(currd, gfn_list[i],
> +                                      _mfn(mfn_list[i]));
>              /* rc should be -EIO for any iteration other than the first */
>              if ( rc && i )
>                  rc = -EIO;
> 
> But the guest_physmap_add_entry() is problematic as it will IOMMU map pages 
> as well, which is probably not wanted.

Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
How about transforming set_foreign_p2m_entry() into
set_special_p2m_entry(), with a type passed in?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 26 September 2018 12:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: IOREQ server on Arm
> 
> >>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1105,8 +1105,11 @@ static int acquire_resource(
> >
> >          for ( i = 0; !rc && i < xmar.nr_frames; i++ )
> >          {
> > -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> > -                                       _mfn(mfn_list[i]));
> > +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
> > +                guest_physmap_add_entry(currd, gfn_list[i],
> > +                                        _mfn(mfn_list[i]), 0,
> p2m_ram_rw) :
> > +                set_foreign_p2m_entry(currd, gfn_list[i],
> > +                                      _mfn(mfn_list[i]));
> >              /* rc should be -EIO for any iteration other than the first
> */
> >              if ( rc && i )
> >                  rc = -EIO;
> >
> > But the guest_physmap_add_entry() is problematic as it will IOMMU map
> pages
> > as well, which is probably not wanted.
> 
> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
> How about transforming set_foreign_p2m_entry() into
> set_special_p2m_entry(), with a type passed in?
> 

That sounds like it might work.

Julien, do you want page types to distinguish caller-owned resources from normal RAM are you ok with p2m_ram_rw even though it could be subject of another domain's foreign map?

  Paul

> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi Paul,

On 09/26/2018 01:01 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 26 September 2018 12:57
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
>> devel@lists.xenproject.org>
>> Subject: RE: IOREQ server on Arm
>>
>>>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
>>>
>>>           for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>>           {
>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>> -                                       _mfn(mfn_list[i]));
>>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
>>> +                guest_physmap_add_entry(currd, gfn_list[i],
>>> +                                        _mfn(mfn_list[i]), 0,
>> p2m_ram_rw) :
>>> +                set_foreign_p2m_entry(currd, gfn_list[i],
>>> +                                      _mfn(mfn_list[i]));
>>>               /* rc should be -EIO for any iteration other than the first
>> */
>>>               if ( rc && i )
>>>                   rc = -EIO;
>>>
>>> But the guest_physmap_add_entry() is problematic as it will IOMMU map
>> pages
>>> as well, which is probably not wanted.
>>
>> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
>> How about transforming set_foreign_p2m_entry() into
>> set_special_p2m_entry(), with a type passed in?
>>
> 
> That sounds like it might work.
> 
> Julien, do you want page types to distinguish caller-owned resources from normal RAM are you ok with p2m_ram_rw even though it could be subject of another domain's foreign map?

Based on your previous e-mail, I would be fine with that on Arm.

This brings me to the next question. Do you expect set_special_p2m_entry 
to take a reference on the page?

If not, we may run into some troubles because AFAICT you can map twice 
the ioreq page in a guest but reference will only be taken on the 
allocation.

However, the unmap path will always drop a reference when removing the 
page. This is because Xen at the moment, reference will not be taken on 
mapping but allocation (we assume a page could not be mapped twice in a 
guest).

Foreign mapping on Arm are a bit special because we get a reference on 
mapping them and will drop it when the mapping disappear. So we would 
not have any problem there.

Any thoughts?

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Jan Beulich]

>>> On 26.09.18 at 23:32, <julien.grall@arm.com> wrote:
> This brings me to the next question. Do you expect set_special_p2m_entry 
> to take a reference on the page?
> 
> If not, we may run into some troubles because AFAICT you can map twice 
> the ioreq page in a guest but reference will only be taken on the 
> allocation.
> 
> However, the unmap path will always drop a reference when removing the 
> page. This is because Xen at the moment, reference will not be taken on 
> mapping but allocation (we assume a page could not be mapped twice in a 
> guest).
> 
> Foreign mapping on Arm are a bit special because we get a reference on 
> mapping them and will drop it when the mapping disappear. So we would 
> not have any problem there.

Well, at least on x86 no refcounting happens at all in the p2m layer.
That's something we mean to fix eventually, but I'm unaware of any
work into that direction. If the situation is different on ARM, then
the behavior of set_special_p2m_entry() may need to differ between
it and x86 for the time being.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 26 September 2018 22:32
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> Hi Paul,
> 
> On 09/26/2018 01:01 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 26 September 2018 12:57
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> >> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> >> devel@lists.xenproject.org>
> >> Subject: RE: IOREQ server on Arm
> >>
> >>>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
> >>>
> >>>           for ( i = 0; !rc && i < xmar.nr_frames; i++ )
> >>>           {
> >>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>> -                                       _mfn(mfn_list[i]));
> >>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
> >>> +                guest_physmap_add_entry(currd, gfn_list[i],
> >>> +                                        _mfn(mfn_list[i]), 0,
> >> p2m_ram_rw) :
> >>> +                set_foreign_p2m_entry(currd, gfn_list[i],
> >>> +                                      _mfn(mfn_list[i]));
> >>>               /* rc should be -EIO for any iteration other than the
> first
> >> */
> >>>               if ( rc && i )
> >>>                   rc = -EIO;
> >>>
> >>> But the guest_physmap_add_entry() is problematic as it will IOMMU map
> >> pages
> >>> as well, which is probably not wanted.
> >>
> >> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
> >> How about transforming set_foreign_p2m_entry() into
> >> set_special_p2m_entry(), with a type passed in?
> >>
> >
> > That sounds like it might work.
> >
> > Julien, do you want page types to distinguish caller-owned resources
> from normal RAM are you ok with p2m_ram_rw even though it could be subject
> of another domain's foreign map?
> 
> Based on your previous e-mail, I would be fine with that on Arm.
> 
> This brings me to the next question. Do you expect set_special_p2m_entry
> to take a reference on the page?
> 
> If not, we may run into some troubles because AFAICT you can map twice
> the ioreq page in a guest but reference will only be taken on the
> allocation.
> 
> However, the unmap path will always drop a reference when removing the
> page. This is because Xen at the moment, reference will not be taken on
> mapping but allocation (we assume a page could not be mapped twice in a
> guest).
> 
> Foreign mapping on Arm are a bit special because we get a reference on
> mapping them and will drop it when the mapping disappear. So we would
> not have any problem there.
> 
> Any thoughts?

Well, as Jan says, on x86 we don't reference count in the P2M so multiple mappings should not be an issue AFAICT. It sounds like resource mapping should be treated the same way as foreign mapping (albeit with a non-foreign domid) such that the reference acquisition occurs at map time.

  Paul

> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi Paul,

On 09/27/2018 09:38 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 26 September 2018 22:32
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
>> <JBeulich@suse.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel <xen-devel@lists.xenproject.org>
>> Subject: Re: IOREQ server on Arm
>>
>> Hi Paul,
>>
>> On 09/26/2018 01:01 PM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: 26 September 2018 12:57
>>>> To: Paul Durrant <Paul.Durrant@citrix.com>
>>>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>>>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>>>> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
>>>> devel@lists.xenproject.org>
>>>> Subject: RE: IOREQ server on Arm
>>>>
>>>>>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
>>>>> --- a/xen/common/memory.c
>>>>> +++ b/xen/common/memory.c
>>>>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
>>>>>
>>>>>            for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>>>>            {
>>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>>>> -                                       _mfn(mfn_list[i]));
>>>>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
>>>>> +                guest_physmap_add_entry(currd, gfn_list[i],
>>>>> +                                        _mfn(mfn_list[i]), 0,
>>>> p2m_ram_rw) :
>>>>> +                set_foreign_p2m_entry(currd, gfn_list[i],
>>>>> +                                      _mfn(mfn_list[i]));
>>>>>                /* rc should be -EIO for any iteration other than the
>> first
>>>> */
>>>>>                if ( rc && i )
>>>>>                    rc = -EIO;
>>>>>
>>>>> But the guest_physmap_add_entry() is problematic as it will IOMMU map
>>>> pages
>>>>> as well, which is probably not wanted.
>>>>
>>>> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
>>>> How about transforming set_foreign_p2m_entry() into
>>>> set_special_p2m_entry(), with a type passed in?
>>>>
>>>
>>> That sounds like it might work.
>>>
>>> Julien, do you want page types to distinguish caller-owned resources
>> from normal RAM are you ok with p2m_ram_rw even though it could be subject
>> of another domain's foreign map?
>>
>> Based on your previous e-mail, I would be fine with that on Arm.
>>
>> This brings me to the next question. Do you expect set_special_p2m_entry
>> to take a reference on the page?
>>
>> If not, we may run into some troubles because AFAICT you can map twice
>> the ioreq page in a guest but reference will only be taken on the
>> allocation.
>>
>> However, the unmap path will always drop a reference when removing the
>> page. This is because Xen at the moment, reference will not be taken on
>> mapping but allocation (we assume a page could not be mapped twice in a
>> guest).
>>
>> Foreign mapping on Arm are a bit special because we get a reference on
>> mapping them and will drop it when the mapping disappear. So we would
>> not have any problem there.
>>
>> Any thoughts?
> 
> Well, as Jan says, on x86 we don't reference count in the P2M so multiple mappings should not be an issue AFAICT.

I understand that you don't have reference count in the P2M (that's the 
same on Arm today except for foreign mapping). But I think I can list at 
least 2 major issues with the design today. Let me give an example based 
on my understanding.

   1. DM requests to map the IOREQ page
	a) page allocated (one reference)
	b) get reference (will be dropped when the IOREQ server is destroyed)

   2. DM requests to map the IOREQ page (second time)
	No reference taken

   3. DM unmap the IOREQ page
   4. DM unmap the IOREQ page

AFAIU, 3. 4. would be done through XENMEM_remove_from_physmap. So no 
reference dropped there. While the reference 1.b) will be dropped in 
hvm_free_ioreq_mfn. AFAICT 1.a) would be kept until the domain die. This 
would result to Xen memory exhaustion in long term. Did I miss anything?

But, I think there are another way for badly written guest to remove the 
page. It looks like you can use XENMEM_decrease_reservation as the page 
belongs to the guest. So a reference would be dropped by 3. and 4.

While 3. will drop the reference drop by 1.a), 4. may drop the reference 
from 1.b) and releasing the page for good. Although the page will still 
be associated with the IOREQ server until it has been effectively 
destroyed. Did I miss anything in the code?

> It sounds like resource mapping should be treated the same way as foreign mapping (albeit with a non-foreign domid) such that the reference acquisition occurs at map time.

If my understanding is correct then yes it would be much safer to get 
reference here.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 27 September 2018 10:42
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> Hi Paul,
> 
> On 09/27/2018 09:38 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@arm.com]
> >> Sent: 26 September 2018 22:32
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> >> <JBeulich@suse.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-
> >> devel <xen-devel@lists.xenproject.org>
> >> Subject: Re: IOREQ server on Arm
> >>
> >> Hi Paul,
> >>
> >> On 09/26/2018 01:01 PM, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>> Sent: 26 September 2018 12:57
> >>>> To: Paul Durrant <Paul.Durrant@citrix.com>
> >>>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >>>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> >>>> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> >>>> devel@lists.xenproject.org>
> >>>> Subject: RE: IOREQ server on Arm
> >>>>
> >>>>>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
> >>>>> --- a/xen/common/memory.c
> >>>>> +++ b/xen/common/memory.c
> >>>>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
> >>>>>
> >>>>>            for ( i = 0; !rc && i < xmar.nr_frames; i++ )
> >>>>>            {
> >>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>>>> -                                       _mfn(mfn_list[i]));
> >>>>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
> >>>>> +                guest_physmap_add_entry(currd, gfn_list[i],
> >>>>> +                                        _mfn(mfn_list[i]), 0,
> >>>> p2m_ram_rw) :
> >>>>> +                set_foreign_p2m_entry(currd, gfn_list[i],
> >>>>> +                                      _mfn(mfn_list[i]));
> >>>>>                /* rc should be -EIO for any iteration other than the
> >> first
> >>>> */
> >>>>>                if ( rc && i )
> >>>>>                    rc = -EIO;
> >>>>>
> >>>>> But the guest_physmap_add_entry() is problematic as it will IOMMU
> map
> >>>> pages
> >>>>> as well, which is probably not wanted.
> >>>>
> >>>> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
> >>>> How about transforming set_foreign_p2m_entry() into
> >>>> set_special_p2m_entry(), with a type passed in?
> >>>>
> >>>
> >>> That sounds like it might work.
> >>>
> >>> Julien, do you want page types to distinguish caller-owned resources
> >> from normal RAM are you ok with p2m_ram_rw even though it could be
> subject
> >> of another domain's foreign map?
> >>
> >> Based on your previous e-mail, I would be fine with that on Arm.
> >>
> >> This brings me to the next question. Do you expect
> set_special_p2m_entry
> >> to take a reference on the page?
> >>
> >> If not, we may run into some troubles because AFAICT you can map twice
> >> the ioreq page in a guest but reference will only be taken on the
> >> allocation.
> >>
> >> However, the unmap path will always drop a reference when removing the
> >> page. This is because Xen at the moment, reference will not be taken on
> >> mapping but allocation (we assume a page could not be mapped twice in a
> >> guest).
> >>
> >> Foreign mapping on Arm are a bit special because we get a reference on
> >> mapping them and will drop it when the mapping disappear. So we would
> >> not have any problem there.
> >>
> >> Any thoughts?
> >
> > Well, as Jan says, on x86 we don't reference count in the P2M so
> multiple mappings should not be an issue AFAICT.
> 
> I understand that you don't have reference count in the P2M (that's the
> same on Arm today except for foreign mapping). But I think I can list at
> least 2 major issues with the design today. Let me give an example based
> on my understanding.
> 
>    1. DM requests to map the IOREQ page
> 	a) page allocated (one reference)
> 	b) get reference (will be dropped when the IOREQ server is
> destroyed)
> 
>    2. DM requests to map the IOREQ page (second time)
> 	No reference taken
> 
>    3. DM unmap the IOREQ page
>    4. DM unmap the IOREQ page
> 
> AFAIU, 3. 4. would be done through XENMEM_remove_from_physmap. So no
> reference dropped there. While the reference 1.b) will be dropped in
> hvm_free_ioreq_mfn. AFAICT 1.a) would be kept until the domain die. This
> would result to Xen memory exhaustion in long term. Did I miss anything?
> 

1.a) would be kept until the IOREQ server is destroyed, which will happen either at domain destruction *or* when the DM destroys it.

> But, I think there are another way for badly written guest to remove the
> page. It looks like you can use XENMEM_decrease_reservation as the page
> belongs to the guest. So a reference would be dropped by 3. and 4.
> 

How so? The pages don't belong to the guest; they belong the the DM domain. They never appear in guest P2M so how can the guest decrease_reservation them? Are you worried about the DM domain doing a decrease reservation?

  Paul

> While 3. will drop the reference drop by 1.a), 4. may drop the reference
> from 1.b) and releasing the page for good. Although the page will still
> be associated with the IOREQ server until it has been effectively
> destroyed. Did I miss anything in the code?
> 
> > It sounds like resource mapping should be treated the same way as
> foreign mapping (albeit with a non-foreign domid) such that the reference
> acquisition occurs at map time.
> 
> If my understanding is correct then yes it would be much safer to get
> reference here.
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi Paul,

On 09/27/2018 11:16 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 27 September 2018 10:42
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
>> <JBeulich@suse.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel <xen-devel@lists.xenproject.org>
>> Subject: Re: IOREQ server on Arm
>>
>> Hi Paul,
>>
>> On 09/27/2018 09:38 AM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall [mailto:julien.grall@arm.com]
>>>> Sent: 26 September 2018 22:32
>>>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
>>>> <JBeulich@suse.com>
>>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>>>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
>> xen-
>>>> devel <xen-devel@lists.xenproject.org>
>>>> Subject: Re: IOREQ server on Arm
>>>>
>>>> Hi Paul,
>>>>
>>>> On 09/26/2018 01:01 PM, Paul Durrant wrote:
>>>>>> -----Original Message-----
>>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>> Sent: 26 September 2018 12:57
>>>>>> To: Paul Durrant <Paul.Durrant@citrix.com>
>>>>>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>>>>>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>>>>>> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
>>>>>> devel@lists.xenproject.org>
>>>>>> Subject: RE: IOREQ server on Arm
>>>>>>
>>>>>>>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
>>>>>>> --- a/xen/common/memory.c
>>>>>>> +++ b/xen/common/memory.c
>>>>>>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
>>>>>>>
>>>>>>>             for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>>>>>>>             {
>>>>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>>>>>> -                                       _mfn(mfn_list[i]));
>>>>>>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
>>>>>>> +                guest_physmap_add_entry(currd, gfn_list[i],
>>>>>>> +                                        _mfn(mfn_list[i]), 0,
>>>>>> p2m_ram_rw) :
>>>>>>> +                set_foreign_p2m_entry(currd, gfn_list[i],
>>>>>>> +                                      _mfn(mfn_list[i]));
>>>>>>>                 /* rc should be -EIO for any iteration other than the
>>>> first
>>>>>> */
>>>>>>>                 if ( rc && i )
>>>>>>>                     rc = -EIO;
>>>>>>>
>>>>>>> But the guest_physmap_add_entry() is problematic as it will IOMMU
>> map
>>>>>> pages
>>>>>>> as well, which is probably not wanted.
>>>>>>
>>>>>> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
>>>>>> How about transforming set_foreign_p2m_entry() into
>>>>>> set_special_p2m_entry(), with a type passed in?
>>>>>>
>>>>>
>>>>> That sounds like it might work.
>>>>>
>>>>> Julien, do you want page types to distinguish caller-owned resources
>>>> from normal RAM are you ok with p2m_ram_rw even though it could be
>> subject
>>>> of another domain's foreign map?
>>>>
>>>> Based on your previous e-mail, I would be fine with that on Arm.
>>>>
>>>> This brings me to the next question. Do you expect
>> set_special_p2m_entry
>>>> to take a reference on the page?
>>>>
>>>> If not, we may run into some troubles because AFAICT you can map twice
>>>> the ioreq page in a guest but reference will only be taken on the
>>>> allocation.
>>>>
>>>> However, the unmap path will always drop a reference when removing the
>>>> page. This is because Xen at the moment, reference will not be taken on
>>>> mapping but allocation (we assume a page could not be mapped twice in a
>>>> guest).
>>>>
>>>> Foreign mapping on Arm are a bit special because we get a reference on
>>>> mapping them and will drop it when the mapping disappear. So we would
>>>> not have any problem there.
>>>>
>>>> Any thoughts?
>>>
>>> Well, as Jan says, on x86 we don't reference count in the P2M so
>> multiple mappings should not be an issue AFAICT.
>>
>> I understand that you don't have reference count in the P2M (that's the
>> same on Arm today except for foreign mapping). But I think I can list at
>> least 2 major issues with the design today. Let me give an example based
>> on my understanding.
>>
>>     1. DM requests to map the IOREQ page
>> 	a) page allocated (one reference)
>> 	b) get reference (will be dropped when the IOREQ server is
>> destroyed)
>>
>>     2. DM requests to map the IOREQ page (second time)
>> 	No reference taken
>>
>>     3. DM unmap the IOREQ page
>>     4. DM unmap the IOREQ page
>>
>> AFAIU, 3. 4. would be done through XENMEM_remove_from_physmap. So no
>> reference dropped there. While the reference 1.b) will be dropped in
>> hvm_free_ioreq_mfn. AFAICT 1.a) would be kept until the domain die. This
>> would result to Xen memory exhaustion in long term. Did I miss anything?
>>
> 
> 1.a) would be kept until the IOREQ server is destroyed, which will happen either at domain destruction *or* when the DM destroys it.

Argh, I mixed get_page_type and get_page(). Sorry for the noise.

> 
>> But, I think there are another way for badly written guest to remove the
>> page. It looks like you can use XENMEM_decrease_reservation as the page
>> belongs to the guest. So a reference would be dropped by 3. and 4.
>>
> 
> How so? The pages don't belong to the guest; they belong the the DM domain. They never appear in guest P2M so how can the guest decrease_reservation them? Are you worried about the DM domain doing a decrease reservation?

By guest I meant DM domain.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 27 September 2018 11:31
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: IOREQ server on Arm
> 
> Hi Paul,
> 
> On 09/27/2018 11:16 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@arm.com]
> >> Sent: 27 September 2018 10:42
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> >> <JBeulich@suse.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-
> >> devel <xen-devel@lists.xenproject.org>
> >> Subject: Re: IOREQ server on Arm
> >>
> >> Hi Paul,
> >>
> >> On 09/27/2018 09:38 AM, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Julien Grall [mailto:julien.grall@arm.com]
> >>>> Sent: 26 September 2018 22:32
> >>>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> >>>> <JBeulich@suse.com>
> >>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >>>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> >> xen-
> >>>> devel <xen-devel@lists.xenproject.org>
> >>>> Subject: Re: IOREQ server on Arm
> >>>>
> >>>> Hi Paul,
> >>>>
> >>>> On 09/26/2018 01:01 PM, Paul Durrant wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>>>> Sent: 26 September 2018 12:57
> >>>>>> To: Paul Durrant <Paul.Durrant@citrix.com>
> >>>>>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >>>>>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>;
> >>>>>> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> >>>>>> devel@lists.xenproject.org>
> >>>>>> Subject: RE: IOREQ server on Arm
> >>>>>>
> >>>>>>>>> On 26.09.18 at 13:02, <Paul.Durrant@citrix.com> wrote:
> >>>>>>> --- a/xen/common/memory.c
> >>>>>>> +++ b/xen/common/memory.c
> >>>>>>> @@ -1105,8 +1105,11 @@ static int acquire_resource(
> >>>>>>>
> >>>>>>>             for ( i = 0; !rc && i < xmar.nr_frames; i++ )
> >>>>>>>             {
> >>>>>>> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>>>>>> -                                       _mfn(mfn_list[i]));
> >>>>>>> +            rc = (xmar.flags & XENMEM_rsrc_acq_caller_owned) ?
> >>>>>>> +                guest_physmap_add_entry(currd, gfn_list[i],
> >>>>>>> +                                        _mfn(mfn_list[i]), 0,
> >>>>>> p2m_ram_rw) :
> >>>>>>> +                set_foreign_p2m_entry(currd, gfn_list[i],
> >>>>>>> +                                      _mfn(mfn_list[i]));
> >>>>>>>                 /* rc should be -EIO for any iteration other than
> the
> >>>> first
> >>>>>> */
> >>>>>>>                 if ( rc && i )
> >>>>>>>                     rc = -EIO;
> >>>>>>>
> >>>>>>> But the guest_physmap_add_entry() is problematic as it will IOMMU
> >> map
> >>>>>> pages
> >>>>>>> as well, which is probably not wanted.
> >>>>>>
> >>>>>> Yeah, I'd prefer if we avoided establishing IOMMU mappings here.
> >>>>>> How about transforming set_foreign_p2m_entry() into
> >>>>>> set_special_p2m_entry(), with a type passed in?
> >>>>>>
> >>>>>
> >>>>> That sounds like it might work.
> >>>>>
> >>>>> Julien, do you want page types to distinguish caller-owned resources
> >>>> from normal RAM are you ok with p2m_ram_rw even though it could be
> >> subject
> >>>> of another domain's foreign map?
> >>>>
> >>>> Based on your previous e-mail, I would be fine with that on Arm.
> >>>>
> >>>> This brings me to the next question. Do you expect
> >> set_special_p2m_entry
> >>>> to take a reference on the page?
> >>>>
> >>>> If not, we may run into some troubles because AFAICT you can map
> twice
> >>>> the ioreq page in a guest but reference will only be taken on the
> >>>> allocation.
> >>>>
> >>>> However, the unmap path will always drop a reference when removing
> the
> >>>> page. This is because Xen at the moment, reference will not be taken
> on
> >>>> mapping but allocation (we assume a page could not be mapped twice in
> a
> >>>> guest).
> >>>>
> >>>> Foreign mapping on Arm are a bit special because we get a reference
> on
> >>>> mapping them and will drop it when the mapping disappear. So we would
> >>>> not have any problem there.
> >>>>
> >>>> Any thoughts?
> >>>
> >>> Well, as Jan says, on x86 we don't reference count in the P2M so
> >> multiple mappings should not be an issue AFAICT.
> >>
> >> I understand that you don't have reference count in the P2M (that's the
> >> same on Arm today except for foreign mapping). But I think I can list
> at
> >> least 2 major issues with the design today. Let me give an example
> based
> >> on my understanding.
> >>
> >>     1. DM requests to map the IOREQ page
> >> 	a) page allocated (one reference)
> >> 	b) get reference (will be dropped when the IOREQ server is
> >> destroyed)
> >>
> >>     2. DM requests to map the IOREQ page (second time)
> >> 	No reference taken
> >>
> >>     3. DM unmap the IOREQ page
> >>     4. DM unmap the IOREQ page
> >>
> >> AFAIU, 3. 4. would be done through XENMEM_remove_from_physmap. So no
> >> reference dropped there. While the reference 1.b) will be dropped in
> >> hvm_free_ioreq_mfn. AFAICT 1.a) would be kept until the domain die.
> This
> >> would result to Xen memory exhaustion in long term. Did I miss
> anything?
> >>
> >
> > 1.a) would be kept until the IOREQ server is destroyed, which will
> happen either at domain destruction *or* when the DM destroys it.
> 
> Argh, I mixed get_page_type and get_page(). Sorry for the noise.
> 
> >
> >> But, I think there are another way for badly written guest to remove
> the
> >> page. It looks like you can use XENMEM_decrease_reservation as the page
> >> belongs to the guest. So a reference would be dropped by 3. and 4.
> >>
> >
> > How so? The pages don't belong to the guest; they belong the the DM
> domain. They never appear in guest P2M so how can the guest
> decrease_reservation them? Are you worried about the DM domain doing a
> decrease reservation?
> 
> By guest I meant DM domain.
> 

If the DM domain is not PV then currently it must be the hardware domain to be able to map resources. Hence we trust it not to descrease_reservation IOREQ pages.

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Julien Grall]

Hi Paul,

Thank you for your help understanding the resource code.

On 09/27/2018 11:46 AM, Paul Durrant wrote:
> If the DM domain is not PV then currently it must be the hardware domain to be able to map resources. Hence we trust it not to descrease_reservation IOREQ pages.

Can you point me to check in the code? The only check I found is:

!(xmar.flags & XENMEM_rsrc_acq_caller_owned) && !is_hardware_domain(d)

But this is still allowing to map some of the resources in a DM domain.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: IOREQ server on Arm

[Roger Pau Monné]

On Wed, Sep 26, 2018 at 11:32:38AM +0100, Julien Grall wrote:
> Hi,
> 
> On 09/26/2018 10:14 AM, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > Sent: 26 September 2018 09:09
> > > To: Julien Grall <julien.grall@arm.com>; Paul Durrant
> > > <Paul.Durrant@citrix.com>
> > > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> > > <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> > > devel <xen-devel@lists.xenproject.org>
> > > Subject: Re: IOREQ server on Arm
> > > 
> > > > > > On 26.09.18 at 00:39, <julien.grall@arm.com> wrote:
> > > > Hi Paul,
> > > > 
> > > > I am looking at porting the IOREQ server infrastructure on Arm. I didn't
> > > > need much modification to make it run for Arm. Although, the
> > > > implementation could be simplified over the x86 implementation.
> > > > 
> > > > I noticed some issue while trying to implement the hypercall
> > > > XENMEM_acquire_resource. Per my understanding, all the page mapped via
> > > > that hypercall will use the type p2m_mapping_foreign.
> > > > 
> > > > This will result to trigger the ASSERT(fdom != dom) in get_page_from_gfn
> > > > (asm-arm/p2m.h) because the IOREQ page has been allocated to the
> > > > emulator domain and mapped to it. AFAICT x86 has the same assert in
> > > > p2m_get_page_from_gfn(...).
> > > > 
> > > > IHMO, the ASSERT makes sense because you are only meant to map page
> > > > belonging to other domain with that type.
> > > > 
> > > > So I am wondering whether IOREQ server running in PVH Dom0 has been
> > > > tested? What would be the best course of action to fix the issue?
> > > 
> > > I think the p2m type needs to be chosen based on
> > > XENMEM_rsrc_acq_caller_owned.
> > > 
> > 
> > Yes, that's correct. There is a FIXME clause in acquire_resource so that that only caller owned resources can be mapped by HVM/PVH domains. Thus the new call can be used for IOREQ server pages, but not grant frames.
> 
> I don't understand your last sentence. IOREQ is caller owned and therefore
> should work.
> 
> As I said, I don't have any problem with mapping the resource. The problem
> is when unmapping it because the region is set with p2m_mapping_foreign. You
> will reach the ASSERT(fdom != p2m->domain) in p2m_get_page_from_gfn.
> 
> Regardless the reference problem (we support it on Arm). Can you explain how
> this is working on PVH Dom0 today?

IOREQs (QEMU) used to work on a PVH Dom0 in the past, it's been some
time since I've launched a HVM guest now, but it definitely worked
before, like it worked for PVHv1 Dom0.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel