Re: [Xen-devel] [PATCH v2 01/25] arm/altp2m: Add first altp2m HVMOP stubs.

[Julien Grall <julien.grall@xxxxxxx>]

On 04/08/16 17:01, Sergej Proskurin wrote:
> Hi Julien,
>
>
> On 08/03/2016 06:54 PM, Julien Grall wrote:
> > Hello Sergej,
> >
> > On 01/08/16 18:10, Sergej Proskurin wrote:
> > > This commit moves the altp2m-related code from x86 to ARM. Functions
> > > that are no yet supported notify the caller or print a BUG message
> > > stating their absence.
> > >
> > > Also, the struct arch_domain is extended with the altp2m_active
> > > attribute, representing the current altp2m activity configuration of the
> > > domain.
> > >
> > > Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> > > ---
> > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Cc: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > > v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M.
> > >     Removed not used altp2m helper stubs in altp2m.h.
> > > ---
> > >  xen/arch/arm/hvm.c           | 79
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  xen/include/asm-arm/altp2m.h |  4 +--
> > >  xen/include/asm-arm/domain.h |  3 ++
> > >  3 files changed, 84 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> > > index d999bde..eb524ae 100644
> > > --- a/xen/arch/arm/hvm.c
> > > +++ b/xen/arch/arm/hvm.c
> > > @@ -32,6 +32,81 @@
> > >
> > >  #include <asm/hypercall.h>
> > >
> > > +#include <asm/altp2m.h>
> > > +
> > > +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
> > > +{
> > > +    struct xen_hvm_altp2m_op a;
> > > +    struct domain *d = NULL;
> > > +    int rc = 0;
> > > +
> > > +    if ( copy_from_guest(&a, arg, 1) )
> > > +        return -EFAULT;
> > > +
> > > +    if ( a.pad1 || a.pad2 ||
> > > +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
> > > +         (a.cmd < HVMOP_altp2m_get_domain_state) ||
> > > +         (a.cmd > HVMOP_altp2m_change_gfn) )
> > > +        return -EINVAL;
> > > +
> > > +    d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ?
> > > +        rcu_lock_domain_by_any_id(a.domain) :
> > > rcu_lock_current_domain();
> > > +
> > > +    if ( d == NULL )
> > > +        return -ESRCH;
> > > +
> > > +    if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
> > > +         (a.cmd != HVMOP_altp2m_set_domain_state) &&
> > > +         !d->arch.altp2m_active )
> > Why not using altp2m_active(d) here?
> I have already changed that within the next patch version. Thank you.
>
> > Also this check looks quite racy. What does prevent another CPU to
> > disable altp2m at the same time? How the code would behave?
> Thank you. I will protect this part with the altp2m_lock.
I have noticed that you use the altp2m_lock (it is a spinlock) in 
multiple places. So you will serialize a lot of code. Is it fine for you?
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel