Re: [PATCH v4 13/25] memory: Add Error** argument to .log_global_start() handler

[Cédric Le Goater]

On 3/15/24 12:18, Peter Xu wrote:
> On Wed, Mar 06, 2024 at 02:34:28PM +0100, Cédric Le Goater wrote:
> > diff --git a/system/memory.c b/system/memory.c
> > index 
> > a229a79988fce2aa3cb77e3a130db4c694e8cd49..3600e716149407c10a1f6bf8f0a81c2611cf15ba
> >  100644
> > --- a/system/memory.c
> > +++ b/system/memory.c
> > @@ -2914,9 +2914,27 @@ static unsigned int postponed_stop_flags;
> >   static VMChangeStateEntry *vmstate_change;
> >   static void memory_global_dirty_log_stop_postponed_run(void);
> >   
> > +/*
> > + * Stop dirty logging on all listeners where it was previously enabled.
> > + */
> > +static void memory_global_dirty_log_rollback(MemoryListener *listener,
> > +                                             unsigned int flags)
> > +{
> > +    global_dirty_tracking &= ~flags;
>
> Having a hook rollback function to touch the global_dirty_tracking flag is
> IMHO tricky.
>
> Can we instead provide a helper to call all log_global_start() hooks, but
> allow a gracefully fail (so rollback will be called if it fails)?
>
>    bool memory_global_dirty_log_start_hooks(...)
>
> Or any better names..  Leaving global_dirty_tracking rollback to
> memory_global_dirty_log_start() when it returns false.
>
> Would this be cleaner?

I will introduce a memory_global_dirty_log_do_start() helper to call
the log_global_start() handlers and to do the rollback in case of
error. Modification of the global_dirty_tracking flag will stay local
to memory_global_dirty_log_start() to avoid any futur errors.


> > +    trace_global_dirty_changed(global_dirty_tracking);
> > +
> > +    while (listener) {
> > +        if (listener->log_global_stop) {
> > +            listener->log_global_stop(listener);
> > +        }
> > +        listener = QTAILQ_PREV(listener, link);
> > +    }
> > +}
> > +
> >   void memory_global_dirty_log_start(unsigned int flags)
> >   {
> >       unsigned int old_flags;
> > +    Error *local_err = NULL;
> >   
> >       assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
> >   
> > @@ -2936,7 +2954,25 @@ void memory_global_dirty_log_start(unsigned int flags)
> >       trace_global_dirty_changed(global_dirty_tracking);
> >   
> >       if (!old_flags) {
> > -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
> > +        MemoryListener *listener;
> > +        bool ret = true;
> > +
> > +        QTAILQ_FOREACH(listener, &memory_listeners, link) {
> > +            if (listener->log_global_start) {
> > +                ret = listener->log_global_start(listener, &local_err);
> > +                if (!ret) {
> > +                    break;
> > +                }
> > +            }
> > +        }
> > +
> > +        if (!ret) {
> > +            memory_global_dirty_log_rollback(QTAILQ_PREV(listener, link),
> > +                                             flags);
> > +            error_report_err(local_err);
> > +            return;
> > +        }
> > +
> >           memory_region_transaction_begin();
> >           memory_region_update_pending = true;
> >           memory_region_transaction_commit();
> > @@ -3009,13 +3045,16 @@ static void listener_add_address_space(MemoryListener 
> > *listener,
> >   {
> >       FlatView *view;
> >       FlatRange *fr;
> > +    Error *local_err = NULL;
> >   
> >       if (listener->begin) {
> >           listener->begin(listener);
> >       }
> >       if (global_dirty_tracking) {
> >           if (listener->log_global_start) {
> > -            listener->log_global_start(listener);
> > +            if (!listener->log_global_start(listener, &local_err)) {
> > +                error_report_err(local_err);
> > +            }
>
> IMHO we should assert here instead of error report.  We have this to guard
> hot-plug during migration so I think the assert is justified:
>
> qdev_device_add_from_qdict():
>
>      if (!migration_is_idle()) {
>          error_setg(errp, "device_add not allowed while migrating");
>          return NULL;
>      }
>
> If it really happens it's a bug, as listener_add_address_space() will still
> keep the rest things around even if the hook failed.  It'll start to be a
> total mess..

OK. I will change the Error parameter to error_abort in that case.

However, It would be useful to catch errors of the .region_add() handler
for VFIO. Let's address that later.

Thanks,

C.