From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events. Date: Thu, 25 Sep 2014 00:07:12 +0200 Message-ID: References: <1411478070-13836-1-git-send-email-tklengyel@sec.in.tum.de> <1411478070-13836-16-git-send-email-tklengyel@sec.in.tum.de> <5422E4AA.4060600@linaro.org> <5422F695.3000809@linaro.org> <54232F23.50205@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5154557138087680674==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: Ian Campbell , Tim Deegan , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org --===============5154557138087680674== Content-Type: multipart/alternative; boundary=001a11c12212e7d13b0503d6e7da --001a11c12212e7d13b0503d6e7da Content-Type: text/plain; charset=ISO-8859-1 On Wed, Sep 24, 2014 at 11:24 PM, Tamas K Lengyel < tamas.lengyel@zentific.com> wrote: > > > On Wed, Sep 24, 2014 at 10:52 PM, Julien Grall > wrote: > >> Hello Tamas, >> >> On 24/09/2014 18:13, Tamas K Lengyel wrote: >> >>> >>> >>> On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall >> > wrote: >>> > >>> > > + && hypercall_preempt_check() ) >>> > > + { >>> > > + rc = progress; >>> > > + goto out; >>> > >>> > Jumping directly to the label "out" will skip flushing the TLB >>> for the >>> > domain. While it wasn't critical until now, partial redo during >>> > insertion/allocation or hypercall preemption only for >>> relinquish, the >>> > guest may use the wrong permission because the TLB hasn't been >>> flushed. >>> > >>> > At the same time, it looks like you never request to flush for >>> the >>> > MEMACCESS operation (see *flush = true). Does memaccess does a >>> TLB flush >>> > somewhere else? >>> > >>> > >>> > Yes, at the end of p2m_set_mem_access once all PTEs are updated >>> > successfully. I guess we could flush the TLB as we are progressing >>> as >>> > well, it wouldn't hurt. >>> >>> We should flush the TLB as we are progressing because the guest may >>> technically continue to run... >>> >>> >>> Hm, I think the guest is always paused while mem_access is being set via >>> memop but sure, it can't hurt. >>> >> >> I didn't find any domain pause call neither in the hypervisor nor in >> xen-access. >> >> >> Unless the pause is done by the hypervisor via the same hypercall, it's >> safer to flush the TLB if it's necessary. >> > > Ack, and you are right, I don't know why I thought it was paused. > > >> >> This case made me also think about another possible issue. Permission >>> are checked in raw_copy_{from,to}_guest_helper during virtual address >>> translation to a physical address. >>> >>> As you modified the attribute in the P2M, the copy may failed >>> because of >>> the lake of permission. >>> >>> >>> I'm not entire sure what you mean. Can you elaborate? >>> >> >> Xen has a bunch of functions raw_copy_{from,to}_guest helpers which copy >> data from/to the guest. >> >> Since XSA-98, Xen checks that the guest has effectively the right to >> read/write (depending of the helpers) a specific mapping before copying the >> data. >> > > Ah yes I remember seeing that, it's passed through get_page_from_gva and > uses the MMU to do the translation directly. > > >> >> If the guest page doesn't have the good right, the helper will fail and >> therefore so do the hypercall. >> >> When memaccess is used, a RAM page may have its permission lower down. >> When the helpers are called, the code to check memory access during >> permission violation will never be called... and the guest will receive an >> hypercall failure. >> > >> This is not the right behavior, the hypercall should only fail if the >> permissions are effectively wrong after check mem access has been called. >> > > Ack, I guess the straight forward solution here would be to forward the > read/write event to the mem_access listener in the inline gvirt_to_maddr > function. For mem_access however I should really include both the gvaddr > and gpaddr values, which means the translation would happen twice, once > with gva_to_ipa (that uses the MMU without the flag-based permission checks > to translate), forward to mem_access, then do the check with the flag-based > permission check again if mem_access is clear. Would that be an acceptable > approach in your opinion? > > So since this the hypervisor that's doing the memory access, we can just fix up the mem_access permissions and forgo sending mem_events. This is absolutely not the best solution and I would prefer in the future to include forwarding these events as well, but based on the discussion about a similar topic recently with Andres and Andrew Cooper ( http://www.gossamer-threads.com/lists/xen/devel/347276) this would be consistent with how such accesses are handled on x86 as well. To actually make the mem_access listener decide whether these accesses should go forward or fail would require some significant more infrastructure, as regular EPT/ARM memory accesses are automatically re-tried after the mem_access listener unpauses the domain. Here, no such luck, we would need to have the hypervisor wait for a reply (maybe with a time-out limit) before it could go forward. Tamas --001a11c12212e7d13b0503d6e7da Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Wed, Sep 24, 2014 at 11:24 PM, Tamas K Lengyel <= ;tamas.leng= yel@zentific.com> wrote:


On Wed, Sep 24, 2014 at 10:52 P= M, Julien Grall <julien.grall@linaro.org> wrote:
Hello Tamas,

On 24/09/2014 18:13, Tamas K Lengyel wrote:


On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall <julien.grall@linaro.org
<= span> <mailto:jul= ien.grall@linaro.org>> wrote:
=A0 =A0 >
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&& = hypercall_preempt_check() )
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 {
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D progres= s;
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
=A0 =A0 >
=A0 =A0 >=A0 =A0 =A0Jumping directly to the label "out" will s= kip flushing the TLB for the
=A0 =A0 >=A0 =A0 =A0domain. While it wasn't critical until now, part= ial redo during
=A0 =A0 >=A0 =A0 =A0insertion/allocation or hypercall preemption only fo= r relinquish, the
=A0 =A0 >=A0 =A0 =A0guest may use the wrong permission because the TLB h= asn't been flushed.
=A0 =A0 >
=A0 =A0 >=A0 =A0 =A0At the same time, it looks like you never request to= flush for the
=A0 =A0 >=A0 =A0 =A0MEMACCESS operation (see *flush =3D true). Does mema= ccess does a TLB flush
=A0 =A0 >=A0 =A0 =A0somewhere else?
=A0 =A0 >
=A0 =A0 >
=A0 =A0 > Yes, at the end of p2m_set_mem_access once all PTEs are update= d
=A0 =A0 > successfully. I guess we could flush the TLB as we are progres= sing as
=A0 =A0 > well, it wouldn't hurt.

=A0 =A0 We should flush the TLB as we are progressing because the guest may=
=A0 =A0 technically continue to run...


Hm, I think the guest is always paused while mem_access is being set via memop but sure, it can't hurt.

I didn't find any domain pause call neither in the hypervisor nor in xe= n-access.


Unless the pause is done by the hypervisor via the same hypercall, it's= safer to flush the TLB if it's necessary.

Ack, and you are right, I don't know w= hy I thought it was paused.
=A0

=A0 =A0 This case made me also think about another possible issue. Permissi= on
=A0 =A0 are checked in raw_copy_{from,to}_guest_helper during virtual addre= ss
=A0 =A0 translation to a physical address.

=A0 =A0 As you modified the attribute in the P2M, the copy may failed becau= se of
=A0 =A0 the lake of permission.


I'm not entire sure what you mean. Can you elaborate?

 Xen has a bunch of functions raw_copy_{from,to}_guest helpers which copy da= ta from/to the guest.

Since XSA-98, Xen checks that the guest has effectively the right to read/w= rite (depending of the helpers) a specific mapping before copying the data.=

Ah yes I remember seeing that, = it's passed through get_page_from_gva and uses the MMU to do the transl= ation directly.
=A0

If the guest page doesn't have the good right, the helper will fail and= therefore so do the hypercall.

When memaccess is used, a RAM page may have its permission lower down.
When the helpers are called, the code to check memory access during permiss= ion violation will never be called... and the guest will receive an hyperca= ll failure.

This is not the right behavior, the hypercall should only fail if the permi= ssions are effectively wrong after check mem access has been called.

Ack, I guess the straight forward solution here w= ould be to forward the read/write event to the mem_access listener in the i= nline gvirt_to_maddr function. For mem_access however I should really inclu= de both the gvaddr and gpaddr values, which means the translation would hap= pen twice, once with gva_to_ipa (that uses the MMU without the flag-based p= ermission checks to translate), forward to mem_access, then do the check wi= th the flag-based permission check again if mem_access is clear. Would that= be an acceptable approach in your opinion?

<= /span>

So since this the h= ypervisor that's doing the memory access, we can just fix up the mem_ac= cess permissions and forgo sending mem_events. This is absolutely not the b= est solution and I would prefer in the future to include forwarding these e= vents as well, but based on the discussion about a similar topic recently w= ith Andres and Andrew Cooper (http://www.gossamer-threads.com/lists/xen/devel/34727= 6) this would be consistent with how such accesses are handled on x86 a= s well.

To actually make the mem_ac= cess listener decide whether these accesses should go forward or fail would= require some significant more infrastructure, as regular EPT/ARM memory ac= cesses are automatically re-tried after the mem_access listener unpauses th= e domain. Here, no such luck, we would need to have the hypervisor wait for= a reply (maybe with a time-out limit) before it could go forward.

Tamas
<= /div>

--001a11c12212e7d13b0503d6e7da-- --===============5154557138087680674== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5154557138087680674==--