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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/04/2016 06:51 PM, Julien Grall wrote:
>
>
> 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).
>

Fair enough. I will go through the locks and think about your
suggestion, thank you.

Best regards,
~Sergej

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