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: Wed, 24 Sep 2014 19:13:29 +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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3929361866875313008==" Return-path: In-Reply-To: <5422F695.3000809@linaro.org> 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 --===============3929361866875313008== Content-Type: multipart/alternative; boundary=089e0149d2e88971010503d2cd73 --089e0149d2e88971010503d2cd73 Content-Type: text/plain; charset=ISO-8859-1 On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall wrote: > Hello Tamas, > > On 09/24/2014 05:27 PM, Tamas K Lengyel wrote: > > > /* Put any references on the single 4K page referenced by pte. > TODO: > > > @@ -553,13 +584,22 @@ static int apply_one_level(struct domain *d, > > > if ( p2m_valid(orig_pte) ) > > > return P2M_ONE_DESCEND; > > > > > > - if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) > ) > > > + if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) > && > > > + /* We only create superpages when mem_access is not in > use. */ > > > + (level == 3 || (level < 3 && !p2m->access_in_use)) ) > > > > Can't this check be moved in is_mapping_aligned? You have nearly the > > same few lines below. > > > > > > Unfortunately not, I already checked and it is used in REMOVE as well in > > which case we would need an exception.. and that wasn't very straight > > forward. > > Ok. > > > > > [..] > > > > > + case MEMACCESS: > > > + if ( level < 3 ) > > > + { > > > + if ( !p2m_valid(orig_pte) ) > > > + { > > > + *addr += level_size; > > > + return P2M_ONE_PROGRESS_NOP; > > > + } > > > + > > > + /* Shatter large pages as we descend */ > > > + if ( p2m_mapping(orig_pte) ) > > > + { > > > + rc = p2m_shatter_page(d, entry, level, > flush_cache); > > > + > > > + if ( rc < 0 ) > > > + return rc; > > > + } /* else: an existing table mapping -> descend */ > > > + > > > + return P2M_ONE_DESCEND; > > > + } > > > + else > > > + { > > > + pte = orig_pte; > > > + > > > + if ( !p2m_table(pte) ) > > > + pte.bits = 0; > > > + > > > + if ( p2m_valid(pte) ) > > > + { > > > + ASSERT(pte.p2m.type != p2m_invalid); > > > > Why the ASSERT? I don't see why we wouldn't want to set permission > for > > this type of page. > > > > > > Not sure, this I copied from p2m_lookup. Can it even happen that > > something passes p2m_valid() but have a type of p2m_invalid? I think > > that just signals that something is very wrong. > > The ASSERT has been added in p2m_lookup, because p2m_invalid means the > MFN is wrong. Hence, p2m_invalid is only used for page table. > > In your case, you don't need to use the MFN. So, IHMO, this ASSERT is > not necessary. > Ack, will remove it. > > > > > > + && 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. > > > [..] > > > > > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const > struct npfec npfec) > > > +{ > > > + int rc; > > > + bool_t violation; > > > + xenmem_access_t xma; > > > + mem_event_request_t *req; > > > + struct vcpu *v = current; > > > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > > > + > > > + /* Mem_access is not in use. */ > > > + if ( !p2m->access_in_use ) > > > + return true; > > > > AFAIU, it's not possible to call this function when mem access is > not in > > use. I would turn this check into an ASSERT. > > > > > > It is possible to call this function when mem_access is not in use and > > it is called every time there is a permission fault in the second stage > > translation. This check here just makes sure the function returns as > > fast as possible when not in use. > > Oh right, sorry for the noise. > > 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? Thanks, Tamas > > Regards, > > -- > Julien Grall > --089e0149d2e88971010503d2cd73 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall <julien.grall@lina= ro.org> wrote:
Hello Tamas,=

On 09/24/2014 05:27 PM, Tamas K Lengyel wrote:
>=A0 =A0 =A0>=A0 /* Put any references on the single 4K page referenc= ed by pte.=A0 TODO:
>=A0 =A0 =A0> @@ -553,13 +584,22 @@ static int apply_one_level(struct= domain *d,
>=A0 =A0 =A0>=A0 =A0 =A0 =A0 =A0 if ( p2m_valid(orig_pte) )
>=A0 =A0 =A0>=A0 =A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_DESCEND;
>=A0 =A0 =A0>
>=A0 =A0 =A0> -=A0 =A0 =A0 =A0 if ( is_mapping_aligned(*addr, end_gpa= ddr, 0, level_size) )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 if ( is_mapping_aligned(*addr, end_gpa= ddr, 0, level_size) &&
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0/* We only create superpages wh= en mem_access is not in use. */
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0(level =3D=3D 3 || (level &= lt; 3 && !p2m->access_in_use)) )
>
>=A0 =A0 =A0Can't this check be moved in is_mapping_aligned? You hav= e nearly the
>=A0 =A0 =A0same few lines below.
>
>
> Unfortunately not, I already checked and it is used in REMOVE as well = in
> which case we would need an exception.. and that wasn't very strai= ght
> forward.

Ok.

>
>=A0 =A0 =A0[..]
>
>=A0 =A0 =A0> +=A0 =A0 case MEMACCESS:
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 if ( level < 3 )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 if ( !p2m_valid(orig_pte) ) >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *addr +=3D level_size;=
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_PROGRES= S_NOP;
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 }
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 /* Shatter large pages as we d= escend */
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 if ( p2m_mapping(orig_pte) ) >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_shatter_pag= e(d, entry, level, flush_cache);
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( rc < 0 )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 } /* else: an existing table m= apping -> descend */
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_DESCEND;
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 }
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 else
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 pte =3D orig_pte;
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 if ( !p2m_table(pte) )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pte.bits =3D 0;
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 if ( p2m_valid(pte) )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ASSERT(pte.p2m.type != =3D p2m_invalid);
>
>=A0 =A0 =A0Why the ASSERT? I don't see why we wouldn't want to = set permission for
>=A0 =A0 =A0this type of page.
>
>
> Not sure, this I copied from p2m_lookup. Can it even happen that
> something passes p2m_valid() but have a type of p2m_invalid? I think > that just signals that something is very wrong.

The ASSERT has been added in p2m_lookup, because p2m_invalid me= ans the
MFN is wrong. Hence, p2m_invalid is only used for page table.

In your case, you don't need to use the MFN. So, IHMO, this ASSERT is not necessary.

Ack, will remove it.
=
=A0

>
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&& hypercal= l_preempt_check() )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D progress;
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
>
>=A0 =A0 =A0Jumping directly to the label "out" will skip flus= hing the TLB for the
>=A0 =A0 =A0domain. While it wasn't critical until now, partial redo= during
>=A0 =A0 =A0insertion/allocation or hypercall preemption only for relinq= uish, the
>=A0 =A0 =A0guest may use the wrong permission because the TLB hasn'= t been flushed.
>
>=A0 =A0 =A0At the same time, it looks like you never request to flush f= or the
>=A0 =A0 =A0MEMACCESS operation (see *flush =3D true). Does memaccess do= es a TLB flush
>=A0 =A0 =A0somewhere 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<= br> > well, it wouldn't hurt.

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

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

>=A0 =A0 =A0[..]
>
>=A0 =A0 =A0> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, = const struct npfec npfec)
>=A0 =A0 =A0> +{
>=A0 =A0 =A0> +=A0 =A0 int rc;
>=A0 =A0 =A0> +=A0 =A0 bool_t violation;
>=A0 =A0 =A0> +=A0 =A0 xenmem_access_t xma;
>=A0 =A0 =A0> +=A0 =A0 mem_event_request_t *req;
>=A0 =A0 =A0> +=A0 =A0 struct vcpu *v =3D current;
>=A0 =A0 =A0> +=A0 =A0 struct p2m_domain *p2m =3D p2m_get_hostp2m(v-&= gt;domain);
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 /* Mem_access is not in use. */
>=A0 =A0 =A0> +=A0 =A0 if ( !p2m->access_in_use )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 return true;
>
>=A0 =A0 =A0AFAIU, it's not possible to call this function when mem = access is not in
>=A0 =A0 =A0use. I would turn this check into an ASSERT.
>
>
> It is possible to call this function when mem_access is not in use and=
> it is called every time there is a permission fault in the second stag= e
> translation. This check here just makes sure the function returns as > fast as possible when not in use.

Oh right, sorry for the noise.

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 ent= ire sure what you mean. Can you elaborate?

Thanks,
Tamas
=A0

Regards,

--
Julien Grall

--089e0149d2e88971010503d2cd73-- --===============3929361866875313008== 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 --===============3929361866875313008==--