Re: [PATCH v3 04/38] arm/p2m: Add first altp2m HVMOP stubs

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,

On 09/02/2016 12:12 PM, Julien Grall wrote:
> 
> 
> On 02/09/16 10:26, Sergej Proskurin wrote:
>> Hi Julien,
> 
> Hello Sergej,
> 
>> On 09/01/2016 06:09 PM, Julien Grall wrote:
>>> Hello Sergej,
>>>
>>> On 16/08/16 23:16, Sergej Proskurin wrote:
>>>> This commit moves the altp2m-related code from x86 to ARM. Functions
>>>
>>> s/moves/copies/
>>>
>>> However, this is not really true because the code is current patch is
>>> not a verbatim copy.
>>>
>>
>> Ok, I will adapt the commit msg to "copies and extends" in the next
>> patch.
>>
>>> Lastly, what is the status to have x86 and ARM implementation of
>>> do_altp2m_op merged?
>>>
>>> It would allow us to get code clean-up more easily. I have in mind the
>>> recent patch [1] from Paul Lai.
>>>
>>> I am also worry to see the code diverging, for instance the locking is
>>> likely needed on x86 too.
>>>
>>
>> We believe that (while merging of both code bases definitely does makes
>> sense) it is out of scope in this patch. The changes you are suggesting
>> would further blow up this patch series. The current patch series is
>> already large enough and we really think we should keep focusing on the
>> implementation the ARM architecture in the first place. We agree that a
>> merge of both architectures is necessary but also strongly believe that
>> the merging should be done in a separate patch.
> 
> That's why you usually have small series to clean-up/move the code
> beforehand. Some of the patch in this series could have been avoided if
> you had a series beforehand to move the x86 code in the common code
> (which is very straight forward).
> 
> My concern here is this code will never get merged and it will continue
> to diverge. A lot of locking issue are also present in x86 path, so I
> don't understand why they should not be fixed now and wait until it get
> merged. So with the plan suggested, ARM and x86 does not benefit of each
> others patches.
> 
> Anyway, I would like to get an action from you to send a series merging
> the two implementation as soon as possible (i.e before Xen 4.9).
> 

I understand your concern and agree on working towards a merged version
between both architectures. Thank you.

>>>> 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@sec.in.tum.de>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>> v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M.
>>>>     Removed not used altp2m helper stubs in altp2m.h.
>>>>
>>>> v3: Cosmetic fixes.
>>>>
>>>>     Added domain lock in "do_altp2m_op" to avoid concurrent
>>>> execution of
>>>>     altp2m-related HVMOPs.
>>>>
>>>>     Added check making sure that HVM_PARAM_ALTP2M is set before
>>>>     execution of altp2m-related HVMOPs.
>>>> ---
>>>>  xen/arch/arm/hvm.c           | 89
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>  xen/include/asm-arm/altp2m.h |  4 +-
>>>>  xen/include/asm-arm/domain.h |  3 ++
>>>>  3 files changed, 94 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>>>> index d999bde..45d51c6 100644
>>>> --- a/xen/arch/arm/hvm.c
>>>> +++ b/xen/arch/arm/hvm.c
>>>> @@ -32,6 +32,91 @@
>>>>
>>>>  #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;
>>>> +
>>>> +    /* Prevent concurrent execution of the following HVMOPs. */
>>>> +    domain_lock(d);
>>>
>>> So you are not allowing concurrent call of set_mem_access on different
>>> altp2m. Is it something that you plan to fix in the future?
>>>
>>
>> Concurrent access of the altp2m interface, including the set_mem_access
>> on different altp2m views, is not performance relevant. However, we will
>> definitely think about which HVMOPs can be executed concurrently without
>> causing troubles.
> 
> It might be worth to add a TODO here.
> 

I will add a TODO stating the upper idea, thank you.

Cheers,
~Sergej

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