Re: [PATCH v5 1/3] xen/riscv: introduce setup_mm()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2024 17:30, oleksii.kurochko@xxxxxxxxx wrote:
> On Thu, 2024-11-14 at 10:49 +0100, Jan Beulich wrote:
>>>>> @@ -423,3 +429,147 @@ void * __init early_fdt_map(paddr_t
>>>>> fdt_paddr)
>>>>>  
>>>>>      return fdt_virt;
>>>>>  }
>>>>> +
>>>>> +vaddr_t __ro_after_init directmap_virt_start =
>>>>> DIRECTMAP_VIRT_START;
>>>>> +
>>>>> +struct page_info *__ro_after_init frametable_virt_start =
>>>>> frame_table;
>>>>> +
>>>>> +#ifndef CONFIG_RISCV_32
>>>>> +
>>>>> +/* Map a frame table to cover physical addresses ps through pe
>>>>> */
>>>>> +static void __init setup_frametable_mappings(paddr_t ps,
>>>>> paddr_t
>>>>> pe)
>>>>> +{
>>>>> +    static mfn_t __initdata frametable_mfn_start =
>>>>> INVALID_MFN_INITIALIZER;
>>>>> +
>>>>> +    paddr_t aligned_ps = ROUNDUP(ps, PAGE_SIZE);
>>>>> +    paddr_t aligned_pe = ROUNDDOWN(pe, PAGE_SIZE);
>>>>> +    unsigned long nr_mfns = PFN_DOWN(aligned_pe - aligned_ps);
>>>>> +    unsigned long frametable_size = nr_mfns *
>>>>> sizeof(*frame_table);
>>>>> +    mfn_t base_mfn;
>>>>> +
>>>>> +    if ( mfn_eq(frametable_mfn_start, INVALID_MFN) )
>>>>> +    {
>>>>> +        frametable_mfn_start = maddr_to_mfn(aligned_ps);
>>>>> +
>>>>> +        frametable_virt_start -= paddr_to_pfn(aligned_ps);
>>>>> +    }
>>>>> +    else
>>>>> +        panic("%s shouldn't be called twice\n", __func__);
>>>>
>>>> As said on the v4 thread - I don't think this is needed. Aiui
>>>> Misra
>>>> would
>>>> actually dislike it, as it's unreachable code. Just to re-
>>>> iterate: My
>>>> complaint there wasn't about this missing check, but about the
>>>> function
>>>> partly giving the impression of expecting to be called more than
>>>> once.
>>> I’ll revert this check, then. Would it be better—and sufficient—to
>>> add
>>> a comment before setup_frametable_mappings() indicating that this
>>> function should only be called once, to avoid any impression that
>>> it
>>> might be expected to be called multiple times?
>>
>> You can add such a comment if you like, we we have many functions of
>> this
>> kind. The important aspect - as said before - is that the function
>> should
>> not - nowhere - give the impression of possibly expecting to be
>> called
>> more than once.
> Then I am not 100% sure how to deal with this impression specifically
> in the case of setup_frametable_mapping() which should be called once.
> 
> The only options I have in my head are:

Let's try to avoid adding such extra baggage. In the v5 form it looked
reasonably okay, iirc.

Jan

>    Option 1:
>        static void __init setup_frametable_mappings(paddr_t ps, paddr_t
>       pe)
>        {
>       +    static bool __read_mostly once;
>       +
>            paddr_t aligned_ps = ROUNDUP(ps, PAGE_SIZE);
>            paddr_t aligned_pe = ROUNDDOWN(pe, PAGE_SIZE);
>            unsigned long nr_mfns = PFN_DOWN(aligned_pe - aligned_ps);
>            unsigned long frametable_size = nr_mfns *
>       sizeof(*frame_table);
>            mfn_t base_mfn;
>        
>       +    ASSERT(!once);
>       +
>       +    once = true;
>       +
>            frametable_virt_start -= paddr_to_pfn(aligned_ps);
>        
>            if ( frametable_size > FRAMETABLE_SIZE )
> 
> 
>    Option 2:
>    
>       -struct page_info *__ro_after_init frametable_virt_start =
>       frame_table;
>       +struct page_info *__ro_after_init frametable_virt_start;
>        
>        #ifndef CONFIG_RISCV_32
>        
>       @@ -442,7 +442,9 @@ static void __init
>       setup_frametable_mappings(paddr_t ps, paddr_t pe)
>            unsigned long frametable_size = nr_mfns *
>    sizeof(*frame_table);
>            mfn_t base_mfn;
>        
>       -    frametable_virt_start -= paddr_to_pfn(aligned_ps);
>       +    ASSERT(!frametable_virt_start);
>       +
>       +    frametable_virt_start = frame_table -
>    paddr_to_pfn(aligned_ps);
>        
>            if ( frametable_size > FRAMETABLE_SIZE )
>                panic("The frametable cannot cover [%#"PRIpaddr",
>       %#"PRIpaddr")\n",
>    
> ~ Oleksii
>