diff mbox series

[RFC,68/84] page_alloc: actually do the mapping and unmapping on xenheap.

Message ID 30e7ab0c4eff006d752ecd004cd1f0130ef52536.1569489002.git.hongyax@amazon.com (mailing list archive)
State New, archived
Headers show
Series Remove direct map from Xen | expand

Commit Message

Xia, Hongyan Sept. 26, 2019, 9:46 a.m. UTC
From: Hongyan Xia <hongyax@amazon.com>

Signed-off-by: Hongyan Xia <hongyax@amazon.com>
---
 xen/common/page_alloc.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Julien Grall Sept. 26, 2019, 10:39 a.m. UTC | #1
Hi,

NIT Title: Please remove full stop.

On 9/26/19 10:46 AM, hongyax@amazon.com wrote:
> From: Hongyan Xia <hongyax@amazon.com>

Please provide a description of what/why you are doing this in the 
commit message.

Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you explain 
why the path with separate xenheap is also modified?

> 
> Signed-off-by: Hongyan Xia <hongyax@amazon.com>
> ---
>   xen/common/page_alloc.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 7cb1bd368b..4ec6299ba8 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>   {
>       struct page_info *pg;
> +    void *ret;
>   
>       ASSERT(!in_irq());
>   
> @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>       if ( unlikely(pg == NULL) )
>           return NULL;
>   
> -    memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
> +    ret = page_to_virt(pg);
> +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
> +                     1UL << order, PAGE_HYPERVISOR);

As mentioned earlier on for Arm, xenheap will always be mapped. So 
unless you have plan to tackle the Arm side as well, we should make sure 
that the behavior is not changed for Arm.

It feels to me we want to introduce a new Kconfig that is selected by 
x86 to tell whether the direct map is mapped. I would then implement 
maybe in xen/mm.h two stub (one for when the config is selected, the 
other when it is not).

>   
>       return page_to_virt(pg);
>   }
> @@ -2165,6 +2169,8 @@ void free_xenheap_pages(void *v, unsigned int order)
>           return;
>   
>       memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
> +    ASSERT((unsigned long)v >= DIRECTMAP_VIRT_START);

This define does not exist for Arm32 so it will break compilation.

> +    map_pages_to_xen((unsigned long)v, INVALID_MFN, 1UL << order, _PAGE_NONE);
>   
>       free_heap_pages(virt_to_page(v), order, false);
>   }
> @@ -2189,6 +2195,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>   {
>       struct page_info *pg;
>       unsigned int i;
> +    void *ret;
>   
>       ASSERT(!in_irq());
>   
> @@ -2204,7 +2211,11 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>       for ( i = 0; i < (1u << order); i++ )
>           pg[i].count_info |= PGC_xen_heap;
>   
> -    return page_to_virt(pg);
> +    ret = page_to_virt(pg);
> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
> +                     1UL << order, PAGE_HYPERVISOR);
> +
> +    return ret;
>   }
>   
>   void free_xenheap_pages(void *v, unsigned int order)
> @@ -2222,6 +2233,9 @@ void free_xenheap_pages(void *v, unsigned int order)
>       for ( i = 0; i < (1u << order); i++ )
>           pg[i].count_info &= ~PGC_xen_heap;
>   
> +    ASSERT((unsigned long)v >= DIRECTMAP_VIRT_START);
> +    map_pages_to_xen((unsigned long)v, INVALID_MFN, 1UL << order, _PAGE_NONE);
> +
>       free_heap_pages(pg, order, true);
>   }
>   
> 

Cheers,
Julien Grall Sept. 26, 2019, 10:45 a.m. UTC | #2
Hi,

On 9/26/19 11:39 AM, Julien Grall wrote:
> Hi,
> 
> NIT Title: Please remove full stop.
> 
> On 9/26/19 10:46 AM, hongyax@amazon.com wrote:
>> From: Hongyan Xia <hongyax@amazon.com>
> 
> Please provide a description of what/why you are doing this in the 
> commit message.
> 
> Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you explain 
> why the path with separate xenheap is also modified?
> 
>>
>> Signed-off-by: Hongyan Xia <hongyax@amazon.com>
>> ---
>>   xen/common/page_alloc.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 7cb1bd368b..4ec6299ba8 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>   {
>>       struct page_info *pg;
>> +    void *ret;
>>       ASSERT(!in_irq());
>> @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int order, 
>> unsigned int memflags)
>>       if ( unlikely(pg == NULL) )
>>           return NULL;
>> -    memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
>> +    ret = page_to_virt(pg);
>> +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
>> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
>> +                     1UL << order, PAGE_HYPERVISOR);
> 
> As mentioned earlier on for Arm, xenheap will always be mapped. So 
> unless you have plan to tackle the Arm side as well, we should make sure 
> that the behavior is not changed for Arm.
> 
> It feels to me we want to introduce a new Kconfig that is selected by 
> x86 to tell whether the direct map is mapped. I would then implement 
> maybe in xen/mm.h two stub (one for when the config is selected, the 
> other when it is not).
> 
>>       return page_to_virt(pg);
>>   }
>> @@ -2165,6 +2169,8 @@ void free_xenheap_pages(void *v, unsigned int 
>> order)
>>           return;
>>       memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
>> +    ASSERT((unsigned long)v >= DIRECTMAP_VIRT_START);
> 
> This define does not exist for Arm32 so it will break compilation.
> 
>> +    map_pages_to_xen((unsigned long)v, INVALID_MFN, 1UL << order, 
>> _PAGE_NONE);
>>       free_heap_pages(virt_to_page(v), order, false);
>>   }
>> @@ -2189,6 +2195,7 @@ void *alloc_xenheap_pages(unsigned int order, 
>> unsigned int memflags)
>>   {
>>       struct page_info *pg;
>>       unsigned int i;
>> +    void *ret;
>>       ASSERT(!in_irq());
>> @@ -2204,7 +2211,11 @@ void *alloc_xenheap_pages(unsigned int order, 
>> unsigned int memflags)
>>       for ( i = 0; i < (1u << order); i++ )
>>           pg[i].count_info |= PGC_xen_heap;
>> -    return page_to_virt(pg);
>> +    ret = page_to_virt(pg);
>> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
>> +                     1UL << order, PAGE_HYPERVISOR);
>> +
>> +    return ret;
>>   }
>>   void free_xenheap_pages(void *v, unsigned int order)
>> @@ -2222,6 +2233,9 @@ void free_xenheap_pages(void *v, unsigned int 
>> order)
>>       for ( i = 0; i < (1u << order); i++ )
>>           pg[i].count_info &= ~PGC_xen_heap;
>> +    ASSERT((unsigned long)v >= DIRECTMAP_VIRT_START);
>> +    map_pages_to_xen((unsigned long)v, INVALID_MFN, 1UL << order, 
>> _PAGE_NONE);
>> +

I forgot to mention one thing. Can you explain why you are using 
map_pages_to_xen and not destroy_xen_mappings here?

 From my understanding, the former will not teardown intermediate 
pagetables. If this is what you want then please write it down in a 
comment. Note that Arm does not support _PAGE_NONE so something similar 
to implementation of vunmap would be necessary.

This might be a call for a helper to do it.

Cheers,
Xia, Hongyan Sept. 26, 2019, 10:52 a.m. UTC | #3
On 26/09/2019 11:45, Julien Grall wrote:
> Hi,
> 
> I forgot to mention one thing. Can you explain why you are using 
> map_pages_to_xen and not destroy_xen_mappings here?
> 
>  From my understanding, the former will not teardown intermediate pagetables. 
> If this is what you want then please write it down in a comment. Note that Arm 
> does not support _PAGE_NONE so something similar to implementation of vunmap 
> would be necessary.

That is exactly my intention. Without an always-mapped direct map, mappings to 
the DIRECTMAP region are created/destroyed alongside xenheap 
allocations/deallocations. Performance-wise it probably makes less sense to 
actually tear down all pagetables and later allocate them again.

I can add a comment.
Xia, Hongyan Sept. 26, 2019, 11:18 a.m. UTC | #4
On 26/09/2019 11:39, Julien Grall wrote:
> Hi,
> 
> NIT Title: Please remove full stop.
> 
> On 9/26/19 10:46 AM, hongyax@amazon.com wrote:
>> From: Hongyan Xia <hongyax@amazon.com>
> 
> Please provide a description of what/why you are doing this in the commit message.
> 
> Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you explain why 
> the path with separate xenheap is also modified?
> 
>>
>> Signed-off-by: Hongyan Xia <hongyax@amazon.com>
>> ---
>>   xen/common/page_alloc.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 7cb1bd368b..4ec6299ba8 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>   {
>>       struct page_info *pg;
>> +    void *ret;
>>       ASSERT(!in_irq());
>> @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int order, unsigned 
>> int memflags)
>>       if ( unlikely(pg == NULL) )
>>           return NULL;
>> -    memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
>> +    ret = page_to_virt(pg);
>> +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
>> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
>> +                     1UL << order, PAGE_HYPERVISOR);
> 
> As mentioned earlier on for Arm, xenheap will always be mapped. So unless you 
> have plan to tackle the Arm side as well, we should make sure that the behavior 
> is not changed for Arm.

I can add an #ifdef for x86. Although I think if the Arm code is correct, this 
should still not break things. It breaks if a xenheap access is made even 
before allocation or after free, which I think is a bug.

> 
> It feels to me we want to introduce a new Kconfig that is selected by x86 to 
> tell whether the direct map is mapped. I would then implement maybe in xen/mm.h 
> two stub (one for when the config is selected, the other when it is not).
>
I have the same question. Do we want to move forward with no direct map in x86 
or do we want to have a compile-time config? If the performance is decent, I 
would prefer the former since this could be a big compile-time switch which 
leaves two branches of code to be maintained in the future.

Hongyan
Julien Grall Sept. 26, 2019, 12:24 p.m. UTC | #5
Hi,

On 9/26/19 12:18 PM, hongyax@amazon.com wrote:
> On 26/09/2019 11:39, Julien Grall wrote:
>> Hi,
>>
>> NIT Title: Please remove full stop.
>>
>> On 9/26/19 10:46 AM, hongyax@amazon.com wrote:
>>> From: Hongyan Xia <hongyax@amazon.com>
>>
>> Please provide a description of what/why you are doing this in the 
>> commit message.
>>
>> Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you 
>> explain why the path with separate xenheap is also modified?
>>
>>>
>>> Signed-off-by: Hongyan Xia <hongyax@amazon.com>
>>> ---
>>>   xen/common/page_alloc.c | 18 ++++++++++++++++--
>>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>> index 7cb1bd368b..4ec6299ba8 100644
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>>   {
>>>       struct page_info *pg;
>>> +    void *ret;
>>>       ASSERT(!in_irq());
>>> @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int order, 
>>> unsigned int memflags)
>>>       if ( unlikely(pg == NULL) )
>>>           return NULL;
>>> -    memguard_unguard_range(page_to_virt(pg), 1 << (order + 
>>> PAGE_SHIFT));
>>> +    ret = page_to_virt(pg);
>>> +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
>>> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
>>> +                     1UL << order, PAGE_HYPERVISOR);
>>
>> As mentioned earlier on for Arm, xenheap will always be mapped. So 
>> unless you have plan to tackle the Arm side as well, we should make 
>> sure that the behavior is not changed for Arm.
> 
> I can add an #ifdef for x86. Although I think if the Arm code is 
> correct, this should still not break things. It breaks if a xenheap 
> access is made even before allocation or after free, which I think is a 
> bug.

Correctness is a matter of perspective ;). xenheap is already map on Arm 
and therefore trying to map it again is considered as an error. I think 
this is a valid behavior because if you try to map twice then it likely 
means you may have to unmap later on.

Furthermore, xenheap is using superpage (2MB, 1GB) mapping at the 
moment. We completely forbid shattering superpage because they are not 
trivial to deal with.

In short, if you wanted to unmap part it, then you would need to shatter 
the page. Shattering superpage required to follow a specific sequence 
(called break-before-make) that will go through an invalid mapping. We 
need to be careful as another processor may access the superpage at the 
same time.

It may be possible to use only 4KB mapping for the xenheap, but that's 
need to be investigated first.

Lastly, not directly related to the discussion here, I think it would be 
a good time to start checking the return of map_pages_to_xen(). If the 
call unlikely fails, we would end up to continue and get an error later 
on that may be more difficult to debug. Instead, I would fail the 
allocation if the mapping is not done.

> 
>>
>> It feels to me we want to introduce a new Kconfig that is selected by 
>> x86 to tell whether the direct map is mapped. I would then implement 
>> maybe in xen/mm.h two stub (one for when the config is selected, the 
>> other when it is not).
>>
> I have the same question. Do we want to move forward with no direct map 
> in x86 or do we want to have a compile-time config? If the performance 
> is decent, I would prefer the former since this could be a big 
> compile-time switch which leaves two branches of code to be maintained 
> in the future.

Unless you have plan to implement the Arm bits, we will need two 
branches to maintain.

But what I suggested is x86 to always select the option that will 
require map/unmap the direct map. Arm would keep it disable.

Cheers,
Xia, Hongyan Sept. 26, 2019, 1:03 p.m. UTC | #6
On 26/09/2019 13:24, Julien Grall wrote:
> Hi,
> 
> On 9/26/19 12:18 PM, hongyax@amazon.com wrote:
>> On 26/09/2019 11:39, Julien Grall wrote:
>>> Hi,
>>>
>>> NIT Title: Please remove full stop.
>>>
>>> On 9/26/19 10:46 AM, hongyax@amazon.com wrote:
>>>> From: Hongyan Xia <hongyax@amazon.com>
>>>
>>> Please provide a description of what/why you are doing this in the commit 
>>> message.
>>>
>>> Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you explain why 
>>> the path with separate xenheap is also modified?
>>>
>>>>
>>>> Signed-off-by: Hongyan Xia <hongyax@amazon.com>
>>>> ---
>>>>   xen/common/page_alloc.c | 18 ++++++++++++++++--
>>>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>>> index 7cb1bd368b..4ec6299ba8 100644
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>>   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>>>   {
>>>>       struct page_info *pg;
>>>> +    void *ret;
>>>>       ASSERT(!in_irq());
>>>> @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int order, 
>>>> unsigned int memflags)
>>>>       if ( unlikely(pg == NULL) )
>>>>           return NULL;
>>>> -    memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
>>>> +    ret = page_to_virt(pg);
>>>> +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
>>>> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
>>>> +                     1UL << order, PAGE_HYPERVISOR);
>>>
>>> As mentioned earlier on for Arm, xenheap will always be mapped. So unless 
>>> you have plan to tackle the Arm side as well, we should make sure that the 
>>> behavior is not changed for Arm.
>>
>> I can add an #ifdef for x86. Although I think if the Arm code is correct, 
>> this should still not break things. It breaks if a xenheap access is made 
>> even before allocation or after free, which I think is a bug.
> 
> Correctness is a matter of perspective ;). xenheap is already map on Arm and 
> therefore trying to map it again is considered as an error. I think this is a 
> valid behavior because if you try to map twice then it likely means you may 
> have to unmap later on.

Good point. Thanks. Will an ifdef do the job?

> 
> Furthermore, xenheap is using superpage (2MB, 1GB) mapping at the moment. We 
> completely forbid shattering superpage because they are not trivial to deal with.
> 
> In short, if you wanted to unmap part it, then you would need to shatter the 
> page. Shattering superpage required to follow a specific sequence (called 
> break-before-make) that will go through an invalid mapping. We need to be 
> careful as another processor may access the superpage at the same time.
> 
> It may be possible to use only 4KB mapping for the xenheap, but that's need to 
> be investigated first.

The series is intended for x86 which then starts with no xenheap mappings. If 
one call to map_pages_to_xen() maps the first half of a superpage and a second 
one maps the remaining, will the second call merge them into an actual 
superpage mapping? Also, do we do a xenheap allocation and then only free part 
of it in some cases? If both answers are hopefully no, then shattering won't 
happen.

> 
> Lastly, not directly related to the discussion here, I think it would be a good 
> time to start checking the return of map_pages_to_xen(). If the call unlikely 
> fails, we would end up to continue and get an error later on that may be more 
> difficult to debug. Instead, I would fail the allocation if the mapping is not 
> done.
> 
>>
>>>
>>> It feels to me we want to introduce a new Kconfig that is selected by x86 to 
>>> tell whether the direct map is mapped. I would then implement maybe in 
>>> xen/mm.h two stub (one for when the config is selected, the other when it is 
>>> not).
>>>
>> I have the same question. Do we want to move forward with no direct map in 
>> x86 or do we want to have a compile-time config? If the performance is 
>> decent, I would prefer the former since this could be a big compile-time 
>> switch which leaves two branches of code to be maintained in the future.
> 
> Unless you have plan to implement the Arm bits, we will need two branches to 
> maintain.
> 
> But what I suggested is x86 to always select the option that will require 
> map/unmap the direct map. Arm would keep it disable.
> 
> Cheers,
> 

Yes, that is what I meant. Sorry if it was not clear. I am happy with an ARM 
branch and an x86 one, but not super happy about x86 separated into two.

Hongyan
Jan Beulich Sept. 26, 2019, 1:22 p.m. UTC | #7
On 26.09.2019 15:03, hongyax@amazon.com wrote:
> The series is intended for x86 which then starts with no xenheap mappings. If 
> one call to map_pages_to_xen() maps the first half of a superpage and a second 
> one maps the remaining, will the second call merge them into an actual 
> superpage mapping?

It will try to, yes.

> Also, do we do a xenheap allocation and then only free part 
> of it in some cases?

We do, yes.

What I'm severely confused by is how you word your questions.
You surely had a need to understand both of these details
before even posting the series, so other than this ...

> If both answers are hopefully no, then shattering won't happen.

... implies you should have known that the answers are "yes"
in both cases.

Jan
Julien Grall Sept. 26, 2019, 2:01 p.m. UTC | #8
Hi,

On 9/26/19 2:03 PM, hongyax@amazon.com wrote:
> On 26/09/2019 13:24, Julien Grall wrote:
>> Hi,
>>
>> On 9/26/19 12:18 PM, hongyax@amazon.com wrote:
>>> On 26/09/2019 11:39, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> NIT Title: Please remove full stop.
>>>>
>>>> On 9/26/19 10:46 AM, hongyax@amazon.com wrote:
>>>>> From: Hongyan Xia <hongyax@amazon.com>
>>>>
>>>> Please provide a description of what/why you are doing this in the 
>>>> commit message.
>>>>
>>>> Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you 
>>>> explain why the path with separate xenheap is also modified?
>>>>
>>>>>
>>>>> Signed-off-by: Hongyan Xia <hongyax@amazon.com>
>>>>> ---
>>>>>   xen/common/page_alloc.c | 18 ++++++++++++++++--
>>>>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>>>> index 7cb1bd368b..4ec6299ba8 100644
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>>>   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>>>>   {
>>>>>       struct page_info *pg;
>>>>> +    void *ret;
>>>>>       ASSERT(!in_irq());
>>>>> @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int 
>>>>> order, unsigned int memflags)
>>>>>       if ( unlikely(pg == NULL) )
>>>>>           return NULL;
>>>>> -    memguard_unguard_range(page_to_virt(pg), 1 << (order + 
>>>>> PAGE_SHIFT));
>>>>> +    ret = page_to_virt(pg);
>>>>> +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
>>>>> +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
>>>>> +                     1UL << order, PAGE_HYPERVISOR);
>>>>
>>>> As mentioned earlier on for Arm, xenheap will always be mapped. So 
>>>> unless you have plan to tackle the Arm side as well, we should make 
>>>> sure that the behavior is not changed for Arm.
>>>
>>> I can add an #ifdef for x86. Although I think if the Arm code is 
>>> correct, this should still not break things. It breaks if a xenheap 
>>> access is made even before allocation or after free, which I think is 
>>> a bug.
>>
>> Correctness is a matter of perspective ;). xenheap is already map on 
>> Arm and therefore trying to map it again is considered as an error. I 
>> think this is a valid behavior because if you try to map twice then it 
>> likely means you may have to unmap later on.
> 
> Good point. Thanks. Will an ifdef do the job?

I would suggest to provide helpers so you can document in a single place 
why this is necessary and avoid to add #ifdefery everywhere.

Also, do we expect similar bits in other part of the common code? If 
yes, I would suggest to add those helpers in the header. If not, 
page_alloc.c should be enough.

Regarding the config name, I would rather not use CONFIG_X86 but use a 
new arch-agnostic one. Maybe CONFIG_DIRECTMAP_NOT_ALWAYS_MAPPED? 
(probably too verbose).

> 
>>
>> Furthermore, xenheap is using superpage (2MB, 1GB) mapping at the 
>> moment. We completely forbid shattering superpage because they are not 
>> trivial to deal with.
>>
>> In short, if you wanted to unmap part it, then you would need to 
>> shatter the page. Shattering superpage required to follow a specific 
>> sequence (called break-before-make) that will go through an invalid 
>> mapping. We need to be careful as another processor may access the 
>> superpage at the same time.
>>
>> It may be possible to use only 4KB mapping for the xenheap, but that's 
>> need to be investigated first.
> 
> The series is intended for x86 which then starts with no xenheap 
> mappings. If one call to map_pages_to_xen() maps the first half of a 
> superpage and a second one maps the remaining, will the second call 
> merge them into an actual superpage mapping? Also, do we do a xenheap 
> allocation and then only free part of it in some cases? If both answers 
> are hopefully no, then shattering won't happen.

For avoidance of doubt, I was describing how Arm works. For any x86 
specific question, then Jan/Andrew are the best point of contact (I saw 
Jan already answered).

The main point is any common code should be able to work for any 
existing architecture (ATM x86, arm)

Cheers,
diff mbox series

Patch

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 7cb1bd368b..4ec6299ba8 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2143,6 +2143,7 @@  void init_xenheap_pages(paddr_t ps, paddr_t pe)
 void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
 {
     struct page_info *pg;
+    void *ret;
 
     ASSERT(!in_irq());
 
@@ -2151,7 +2152,10 @@  void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
     if ( unlikely(pg == NULL) )
         return NULL;
 
-    memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
+    ret = page_to_virt(pg);
+    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
+    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
+                     1UL << order, PAGE_HYPERVISOR);
 
     return page_to_virt(pg);
 }
@@ -2165,6 +2169,8 @@  void free_xenheap_pages(void *v, unsigned int order)
         return;
 
     memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
+    ASSERT((unsigned long)v >= DIRECTMAP_VIRT_START);
+    map_pages_to_xen((unsigned long)v, INVALID_MFN, 1UL << order, _PAGE_NONE);
 
     free_heap_pages(virt_to_page(v), order, false);
 }
@@ -2189,6 +2195,7 @@  void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
 {
     struct page_info *pg;
     unsigned int i;
+    void *ret;
 
     ASSERT(!in_irq());
 
@@ -2204,7 +2211,11 @@  void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
     for ( i = 0; i < (1u << order); i++ )
         pg[i].count_info |= PGC_xen_heap;
 
-    return page_to_virt(pg);
+    ret = page_to_virt(pg);
+    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
+                     1UL << order, PAGE_HYPERVISOR);
+
+    return ret;
 }
 
 void free_xenheap_pages(void *v, unsigned int order)
@@ -2222,6 +2233,9 @@  void free_xenheap_pages(void *v, unsigned int order)
     for ( i = 0; i < (1u << order); i++ )
         pg[i].count_info &= ~PGC_xen_heap;
 
+    ASSERT((unsigned long)v >= DIRECTMAP_VIRT_START);
+    map_pages_to_xen((unsigned long)v, INVALID_MFN, 1UL << order, _PAGE_NONE);
+
     free_heap_pages(pg, order, true);
 }