From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v2 01/25] arm/altp2m: Add first altp2m HVMOP stubs. Date: Wed, 10 Aug 2016 08:49:01 -0600 Message-ID: References: <20160801171028.11615-1-proskurin@sec.in.tum.de> <20160801171028.11615-2-proskurin@sec.in.tum.de> <25ac8782-4a66-68d9-7312-7a16b4cba5d5@arm.com> <9393bc2f-1fee-3946-6c6f-c2c9cdedb3ef@arm.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3433121962210067548==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bXUoa-0004s9-Ll for xen-devel@lists.xenproject.org; Wed, 10 Aug 2016 14:49:04 +0000 Received: by mail-wm0-f67.google.com with SMTP id o80so10022726wme.0 for ; Wed, 10 Aug 2016 07:49:02 -0700 (PDT) In-Reply-To: <9393bc2f-1fee-3946-6c6f-c2c9cdedb3ef@arm.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Julien Grall Cc: Sergej Proskurin , Stefano Stabellini , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============3433121962210067548== Content-Type: multipart/alternative; boundary=001a114b7022102e530539b8c028 --001a114b7022102e530539b8c028 Content-Type: text/plain; charset=UTF-8 On Aug 10, 2016 03:52, "Julien Grall" wrote: > > Hello Tamas, > > > On 09/08/2016 21:16, Tamas K Lengyel wrote: >> >> On Wed, Aug 3, 2016 at 10:54 AM, 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 >>>> --- >>>> Cc: Stefano Stabellini >>>> Cc: Julien Grall >>>> --- >>>> 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 >>>> >>>> +#include >>>> + >>>> +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? >>> >>> Also this check looks quite racy. What does prevent another CPU to disable >>> altp2m at the same time? How the code would behave? >> >> >> There is a rcu_lock_domain_by_any_id before we get to this check here, >> so any other CPU looking to disable altp2m would be waiting there for >> the current op to finish up, so there is no race condition AFAICT. > > > No, rcu_lock_domain_by_any_id only prevents the domain to be fully destroyed by "locking" the rcu. It does not prevent multiple concurrent access. You can look at the code if you are not convinced. > Ah thanks for clarifying. Then indeed there could be concurrency issues if there are multiple tools accessing this interface. Normally that doesn't happen though but probably a good idea to enforce it anyway. Thanks, Tamas --001a114b7022102e530539b8c028 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Aug 10, 2016 03:52, "Julien Grall" <julien.grall@arm.com> wrote:
>
> Hello Tamas,
>
>
> On 09/08/2016 21:16, Tamas K Lengyel wrote:
>>
>> On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com> 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_a= ctive
>>>> attribute, representing the current altp2m activity config= uration 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.
>>>> =C2=A0 =C2=A0 Removed not used altp2m helper stubs in altp= 2m.h.
>>>> ---
>>>> =C2=A0xen/arch/arm/hvm.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0| 79
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>> =C2=A0xen/include/asm-arm/altp2m.h |=C2=A0 4 +--
>>>> =C2=A0xen/include/asm-arm/domain.h |=C2=A0 3 ++
>>>> =C2=A03 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 @@
>>>>
>>>> =C2=A0#include <asm/hypercall.h>
>>>>
>>>> +#include <asm/altp2m.h>
>>>> +
>>>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)=
>>>> +{
>>>> +=C2=A0 =C2=A0 struct xen_hvm_altp2m_op a;
>>>> +=C2=A0 =C2=A0 struct domain *d =3D NULL;
>>>> +=C2=A0 =C2=A0 int rc =3D 0;
>>>> +
>>>> +=C2=A0 =C2=A0 if ( copy_from_guest(&a, arg, 1) )
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
>>>> +
>>>> +=C2=A0 =C2=A0 if ( a.pad1 || a.pad2 ||
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(a.version !=3D HVMOP_A= LTP2M_INTERFACE_VERSION) ||
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(a.cmd < HVMOP_altp2= m_get_domain_state) ||
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(a.cmd > HVMOP_altp2= m_change_gfn) )
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL;
>>>> +
>>>> +=C2=A0 =C2=A0 d =3D (a.cmd !=3D HVMOP_altp2m_vcpu_enable_= notify) ?
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rcu_lock_domain_by_any_id(a.d= omain) : rcu_lock_current_domain();
>>>> +
>>>> +=C2=A0 =C2=A0 if ( d =3D=3D NULL )
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ESRCH;
>>>> +
>>>> +=C2=A0 =C2=A0 if ( (a.cmd !=3D HVMOP_altp2m_get_domain_st= ate) &&
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(a.cmd !=3D HVMOP_altp2= m_set_domain_state) &&
>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0!d->arch.altp2m_acti= ve )
>>>
>>>
>>>
>>> Why not using altp2m_active(d) here?
>>>
>>> Also this check looks quite racy. What does prevent another CP= U to disable
>>> altp2m at the same time? How the code would behave?
>>
>>
>> There is a rcu_lock_domain_by_any_id before we get to this check h= ere,
>> so any other CPU looking to disable altp2m would be waiting there = for
>> the current op to finish up, so there is no race condition AFAICT.=
>
>
> No, rcu_lock_domain_by_any_id only prevents the domain to be fully des= troyed by "locking" the rcu. It does not prevent multiple concurr= ent access. You can look at the code if you are not convinced.
>

Ah thanks for clarifying. Then indeed there could be concurr= ency issues if there are multiple tools accessing this interface. Normally = that doesn't happen though but probably a good idea to enforce it anywa= y.

Thanks,
Tamas

--001a114b7022102e530539b8c028-- --===============3433121962210067548== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============3433121962210067548==--