On 14.11.2024 17:49, oleksii.kuroc...@gmail.com wrote:
> On Thu, 2024-11-14 at 17:30 +0100, oleksii.kuroc...@gmail.com 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:
>> 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",
>>
> Or your intention was that I can drop any check at all in
> setup_frametable_mappings():
> static void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
> {
> 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;
>
> frametable_virt_start -= paddr_to_pfn(aligned_ps);
>
> ...
> }
>
> and it should be enough as this function by its nature shouldn't be called
> twice.
Indeed, that's what I was after.
Jan
