Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Jan Beulich <jbeulich@suse.com>]

On 08.07.2021 11:09, Penny Zheng wrote:
> Hi Jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, July 6, 2021 2:54 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
>> alloc_domstatic_pages
>>
>> On 06.07.2021 07:58, Penny Zheng wrote:
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Thursday, June 10, 2021 6:23 PM
>>>>
>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>>>>>      return pg;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/*
>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
>>>>> + * It is the equivalent of alloc_heap_pages for static memory  */
>>>>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns,
>>>>> +                                               mfn_t smfn,
>>>>> +                                               unsigned int
>>>>> +memflags) {
>>>>> +    bool need_tlbflush = false;
>>>>> +    uint32_t tlbflush_timestamp = 0;
>>>>> +    unsigned long i;
>>>>> +    struct page_info *pg;
>>>>> +
>>>>> +    /* For now, it only supports allocating at specified address. */
>>>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>>>> +    {
>>>>> +        printk(XENLOG_ERR
>>>>> +               "Invalid %lu static memory starting at
>>>>> + %"PRI_mfn"\n",
>>>>
>>>> Reading a log containing e.g. "Invalid 0 static memory starting at
>>>> ..." I don't think I would recognize that the "0" is the count of pages.
>>>
>>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
>>
>> This still doesn't convey _both_ parts of the if() that would cause the log
>> message to be issued.
>>
> 
> Sorry. How about
> "
>         printk(XENLOG_ERR
>                "Either out-of-range static memory starting at %"PRI_mfn""
>                "or invalid number of pages: %ul.\n",
>                mfn_x(smfn), nr_mfns);
> "

I'm sorry - while now you convey both aspects, the message has become
too verbose. What's wrong with "Invalid static memory request: ... pages
at ...\"? But I wonder anyway if a log message is appropriate here in
the first place.

>>>>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
>>>>>      return pg;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/*
>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static
>>>>> +memory,
>>>>> + * then assign them to one specific domain #d.
>>>>> + * It is the equivalent of alloc_domheap_pages for static memory.
>>>>> + */
>>>>> +struct page_info *alloc_domstatic_pages(
>>>>> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
>>>>> +        unsigned int memflags)
>>>>> +{
>>>>> +    struct page_info *pg = NULL;
>>>>> +    unsigned long dma_size;
>>>>> +
>>>>> +    ASSERT(!in_irq());
>>>>> +
>>>>> +    if ( !dma_bitsize )
>>>>> +        memflags &= ~MEMF_no_dma;
>>>>> +    else
>>>>> +    {
>>>>> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
>>>>> +        {
>>>>> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
>>>>> +            /* Starting address shall meet the DMA limitation. */
>>>>> +            if ( mfn_x(smfn) < dma_size )
>>>>> +                return NULL;
>>>>
>>>> I think I did ask this on v1 already: Why the first page? Static
>>>> memory regions, unlike buddy allocator zones, can cross power-of-2
>>>> address boundaries. Hence it ought to be the last page that gets
>>>> checked for fitting address width restriction requirements.
>>>>
>>>> And then - is this necessary at all? Shouldn't "pre-defined by
>>>> configuration using physical address ranges" imply the memory
>>>> designated for a guest fits its DMA needs?
>>>>
>>>
>>> Hmmm, In my understanding, here is the DMA restriction when using buddy
>> allocator:
>>>     else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
>>>         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags,
>>> d); dma_zone is restricting the starting buddy allocator zone, so I am
>>> thinking that here, it shall restrict the first page.
>>>
>>> imo, if let user define, it also could be missing DMA restriction?
>>
>> Did you read my earlier reply? Again: The difference is that ordinary
>> allocations (buddies) can't cross zone boundaries. Hence it is irrelevant if you
>> check DMA properties on the first or last page - both will have the same
>> number of significant bits. The same is - afaict - not true for static allocation
>> ranges.
>>
> 
> True.
> 
> Ordinary allocations (buddies) can't cross zone boundaries, So I understand that
> following the logic in "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);"
> pages of the smallest address shall be allocated from "dma_zone + 1", like you
> said, it is irrelevant if you check DMA properties on the first or last pages, but imo, no matter
> first or last page, both shall be larger than (2^(dma_zone + 1)).
> 
> Taking 32 as dma_bitsize, then the memory with this DMA restriction allocated by
> "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" shall be at least
> more than 4G.

DMA restrictions are always "needs to be no more than N bits".

> That’s why I keep comparing the first page of static allocation, that I am following the
> "more than" logic here.
> 
> But you're right, I got a little investigation on ARM DMA limitation, still taking dma_bitsize=32
> as an example, we want that the actually allocated memory is smaller than 4G, not more than.
> So I think the logic behind this code line
> " alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" is not right for ARM, and it shall
> be changed to "alloc_heap_pages(zone_lo, dma_zone + 1, order, memflags, d);" as correction.

But this step is to _avoid_ the DMA-reserved part of the heap.
The caller requests address restricted memory by passing suitable
memflags. If the request doesn't require access to the DMA-
reserved part of the heap (dma_zone < zone_hi) we first try to
get memory from there. Only if that fails will we fall back and
try taking memory from the lower region. IOW the problem with
your code is more fundamental: You use dma_bitsize when really
you ought to extract the caller's requested restriction (if any)
from memflags. I would assume that dma_bitsize is orthogonal to
static memory, i.e. you don't need to try to preserve low memory.

Jan