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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

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?

Best regards,
~Sergej


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