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

[oleksii.kurochko@xxxxxxxxx]

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",
   
~ Oleksii