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

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-11-14 at 17:30 +0100, 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:
>    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.

~ Oleksii