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: Thu, 11 Aug 2016 08:41:48 -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> <4c38b85c-a3ff-f884-ad43-89aafdd12ca8@arm.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2080343621911363677==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bXrBA-0002qP-9V for xen-devel@lists.xenproject.org; Thu, 11 Aug 2016 14:41:52 +0000 Received: by mail-wm0-f66.google.com with SMTP id i5so198284wmg.2 for ; Thu, 11 Aug 2016 07:41:50 -0700 (PDT) In-Reply-To: <4c38b85c-a3ff-f884-ad43-89aafdd12ca8@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 --===============2080343621911363677== Content-Type: multipart/alternative; boundary=001a113640c61f70ab0539ccc453 --001a113640c61f70ab0539ccc453 Content-Type: text/plain; charset=UTF-8 On Aug 11, 2016 02:18, "Julien Grall" wrote: > > Hello Tamas, > > > On 10/08/2016 16:49, Tamas K Lengyel wrote: >> >> On Aug 10, 2016 03:52, "Julien Grall" > > wrote: >>> >>> On 09/08/2016 21:16, Tamas K Lengyel wrote: >>>> >>>> On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall > >> > wrote: >>>> >>>> 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. > > > Well, you need to think about the worst case scenario when you implement an interface. If you don't lock properly, the state in Xen may be corrupted. For instance Xen may think altp2m is active whilst it is not properly initialized. > Sure. We largely followed the x86 implementation here and there aren't any hvmops there that do synchronization like that, only the rcu lock is taken. Adding a domain_lock() should be fine though. Tamas --001a113640c61f70ab0539ccc453 Content-Type: text/html; charset=UTF-8

On Aug 11, 2016 02:18, "Julien Grall" <julien.grall@arm.com> wrote:
>
> Hello Tamas,
>
>
> On 10/08/2016 16:49, Tamas K Lengyel wrote:
>>
>> On Aug 10, 2016 03:52, "Julien Grall" <julien.grall@arm.com
>> <mailto:julien.grall@arm.com>> wrote:
>>>
>>> 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
>>
>> <mailto:julien.grall@arm.com>> wrote:
>>>>
>>>> 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.
>
>
> Well, you need to think about the worst case scenario when you implement an interface. If you don't lock properly, the state in Xen may be corrupted. For instance Xen may think altp2m is active whilst it is not properly initialized.
>

Sure. We largely followed the x86 implementation here and there aren't any hvmops there that do synchronization like that, only the rcu lock is taken. Adding a domain_lock() should be fine though.

Tamas

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