Re: [PATCH v2] xen/arm: Allow balooning working with 1:1 memory mapping

[Julien Grall <julien.grall@linaro.org>]

On 12/16/2013 09:08 AM, Jan Beulich wrote:
>>>> On 13.12.13 at 21:18, Julien Grall <julien.grall@linaro.org> wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -90,7 +90,7 @@ static void increase_reservation(struct memop_args *a)
>>
>>   static void populate_physmap(struct memop_args *a)
>>   {
>> -    struct page_info *page;
>> +    struct page_info *page = NULL;
>
> Why?

Spurious line from a previous version. I will remove it.

>
>> @@ -122,7 +122,29 @@ static void populate_physmap(struct memop_args *a)
>>           }
>>           else
>>           {
>> -            page = alloc_domheap_pages(d, a->extent_order, a->memflags);
>> +            if ( d == dom0 && is_dom0_mapped_11() )
>> +            {
>> +                mfn = gpfn;
>> +                if (!mfn_valid(mfn))
>
> Coding style.
>
>> +                {
>> +                    gdprintk(XENLOG_INFO, "Invalid mfn 0x%"PRI_xen_pfn"\n",
>> +                             mfn);
>> +                    goto out;
>> +                }
>> +
>> +                page = mfn_to_page(mfn);
>> +                if ( !get_page(page, d) )
>> +                {
>> +                    gdprintk(XENLOG_INFO,
>> +                             "mfn 0x%"PRI_xen_pfn" doesn't belong to dom0\n",
>> +                             mfn);
>> +                    goto out;
>> +                }
>> +                put_page(page);
>> +            }
>
> I think this hack doesn't belong into a common file. Rather than
> having the (anyway oddly named) is_dom0_mapped_11(), it
> would seem more clean to have this implemented by an inline
> function for ARM, returning the struct page_info * (and getting
> #define-d to NULL for all other cases, perhaps not even in an
> x86 header, but right in the source file).
>
> And even if we were to stay with the implementation here, for
> being at least half way sane the checking macro should take a
> domain argument rather than requiring the extra "d == dom0"
> check up front. Plus the "11" there is bogus - I first read this as
> "eleven" rather than "1:1", wondering what eleven here was
> about. I'd suggest something like is_domain_direct_mapped() or
> is_domain_identity_mapped().
>
> Jan
>

-- 
Julien Grall