Re: [PATCH v2 1/2] exec: Check Xen is enabled before calling the Xen API

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

Hi Juan,

On 5/8/20 10:39 AM, Juan Quintela wrote:
> Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> > ---
> >   include/exec/ram_addr.h | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index 5e59a3d8d7..dd8713179e 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -330,7 +330,9 @@ static inline void 
> > cpu_physical_memory_set_dirty_range(ram_addr_t start,
> >           }
> >       }
> >   
> > -    xen_hvm_modified_memory(start, length);
> > +    if (xen_enabled()) {
> > +        xen_hvm_modified_memory(start, length);
> > +    }
> >   }
> >   
> >   #if !defined(_WIN32)
> > @@ -388,7 +390,9 @@ static inline void 
> > cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> >               }
> >           }
> >   
> > -        xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
> > +        if (xen_enabled()) {
> > +            xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
> > +        }
> >       } else {
> >           uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : 
> > DIRTY_CLIENTS_NOCODE;
> I don't object moving the xen code to accell.  But I think that this
> change is bad.
>
> On the following patch:
> - You export xen_allowed
>    (ok, it was already exported, but I think it shouldn't)
>
> (master)$ find . -type f | xargs grep xen_allowed
> ./hw/xen/xen-common.c:    ac->allowed = &xen_allowed;
> ./include/hw/xen/xen.h:extern bool xen_allowed;
> ./include/hw/xen/xen.h:    return xen_allowed;
> ./softmmu/vl.c:bool xen_allowed;
>
> This are all the users that I can find.
>
> And xen_havm_modified_memory() is an empty function if xen is not
> compiled in.  And in the case that xen is compiled in, the 1st thing
> that it checks is:
>
>     if (unlikely(xen_in_migration)) {
>
> That is way more restrictive that xen_enabled().
>
> So, I think that it is better to drop this patch, maintain next one, but
> just un-exporting xen_allowed.
>
> What do you think?
I blindly trust your judgement on this :) I'd rather not touch this code 
but as it happens to be in "exec/ram_addr.h" I had to modify it.
Thanks for your reviews!

> Later, Juan.