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

[Julien Grall <julien.grall@xxxxxxx>]

On 04/08/16 17:22, Sergej Proskurin wrote:
> On 08/04/2016 06:04 PM, Julien Grall wrote:
> > 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?
> I would need to move the lock from altp2m_init_by_id to the outside.
> This would not lock much more code as it already does. Apart from that,
> since activating/deactivating altp2m on a specific domain should be used
> very rarely (including the first time when no altp2m structures are
> initialized), it is fine to me. Unless, you would like me to use a
> different lock instead?
I don't know, it was an open question. There are a couple of places 
where you may need to add lock atlp2m_lock (such altp2m_copy_lazy), so 
you would serialize all the access. If you care about performance, then 
you may want to use a different lock or method of locking (such as 
read-write-lock).
Regards,

--
Julien Grall

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