Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Dec 05, 2023 at 02:47:56PM +0100, Jan Beulich wrote:
> On 30.11.2023 15:29, Roger Pau Monne wrote:
> > diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> > index 5f89703f513b..b444253848cf 100644
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -23,14 +23,8 @@ static struct virtual_region core_init __initdata = {
> >  };
> >  
> >  /*
> > - * RCU locking. Additions are done either at startup (when there is only
> > - * one CPU) or when all CPUs are running without IRQs.
> > - *
> > - * Deletions are bit tricky. We do it when Live Patch (all CPUs running
> > - * without IRQs) or during bootup (when clearing the init).
> > - *
> > - * Hence we use list_del_rcu (which sports an memory fence) and a spinlock
> > - * on deletion.
> > + * RCU locking. Modifications to the list must be done in exclusive mode, and
> > + * hence need to hold the spinlock.
> >   *
> >   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
> >   */
> > @@ -58,38 +52,28 @@ const struct virtual_region *find_text_region(unsigned long addr)
> >  
> >  void register_virtual_region(struct virtual_region *r)
> >  {
> > -    ASSERT(!local_irq_is_enabled());
> > +    unsigned long flags;
> >  
> > +    spin_lock_irqsave(&virtual_region_lock, flags);
> >      list_add_tail_rcu(&r->list, &virtual_region_list);
> > +    spin_unlock_irqrestore(&virtual_region_lock, flags);
> >  }
> >  
> >  static void remove_virtual_region(struct virtual_region *r)
> >  {
> > -    unsigned long flags;
> > +     unsigned long flags;
> 
> Nit: Stray blank added?

Oh, my bad.

> > -    spin_lock_irqsave(&virtual_region_lock, flags);
> > -    list_del_rcu(&r->list);
> > -    spin_unlock_irqrestore(&virtual_region_lock, flags);
> > -    /*
> > -     * We do not need to invoke call_rcu.
> > -     *
> > -     * This is due to the fact that on the deletion we have made sure
> > -     * to use spinlocks (to guard against somebody else calling
> > -     * unregister_virtual_region) and list_deletion spiced with
> > -     * memory barrier.
> > -     *
> > -     * That protects us from corrupting the list as the readers all
> > -     * use list_for_each_entry_rcu which is safe against concurrent
> > -     * deletions.
> > -     */
> > +     spin_lock_irqsave(&virtual_region_lock, flags);
> > +     list_del_rcu(&r->list);
> > +     spin_unlock_irqrestore(&virtual_region_lock, flags);
> >  }
> >  
> >  void unregister_virtual_region(struct virtual_region *r)
> >  {
> > -    /* Expected to be called from Live Patch - which has IRQs disabled. */
> > -    ASSERT(!local_irq_is_enabled());
> > -
> >      remove_virtual_region(r);
> > +
> > +    /* Assert that no CPU might be using the removed region. */
> > +    rcu_barrier();
> >  }
> 
> rcu_barrier() is a relatively heavy operation aiui. Seeing ...
> 
> >  #if defined(CONFIG_LIVEPATCH) && defined(CONFIG_X86)
> 
> ... this I'd like to ask to consider hiding {,un}register_virtual_region()
> in "#ifdef CONFIG_LIVEPATCH" as well, to make clear these aren't supposed
> to be used for other purpose. Would at the same time address two Misra
> violations, I think (both functions having no callers when !LIVEPATCH).

That's fine, I can do it this same patch unless you prefer such
adjustment to be in a separate change.

Thanks, Roger.