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 18:27:16 +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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4629193160327628519==" Return-path: In-Reply-To: <5422E4AA.4060600@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 --===============4629193160327628519== Content-Type: multipart/alternative; boundary=001a113a96ca3b5cc30503d22839 --001a113a96ca3b5cc30503d22839 Content-Type: text/plain; charset=ISO-8859-1 On Wed, Sep 24, 2014 at 5:35 PM, Julien Grall wrote: > Hello Tamas, > > On 09/23/2014 02:14 PM, Tamas K Lengyel wrote: > > This patch enables to store, set, check and deliver LPAE R/W mem_events. > > As the LPAE PTE's lack enough available software programmable bits, > > we store the permissions in a Radix tree. A custom boolean, > access_in_use, > > specifies if the tree is in use to avoid uneccessary lookups on an empty > tree. > > unecessary > Ack. > [..] > > > +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned > long pfn, > > Shouldn't "int" enough for the return type? > > > + p2m_access_t a) > > +{ > > + long rc; > > NIT: missing new line here. > Ack. > > [..] > > > /* 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. > > [..] > > > + 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. > > [..] > > > @@ -821,6 +912,21 @@ static int apply_p2m_changes(struct domain *d, > > count = 0; > > } > > > > + /* > > + * Preempt setting mem_access permissions as required by XSA-89, > > + * if it's not the last iteration. > > + */ > > + if ( op == MEMACCESS && count ) > > + { > > + int progress = paddr_to_pfn(addr) - start_gpfn + 1; > > uint32_t? > Ack. > > > NIT: Missing blank line. > > > + if ( (end_gpfn-start_gpfn) > progress && !(progress & mask) > > NIT: (end_gpfn - start_gpfn) > > Also you are comparing with an "int" with an "unsigned long". I'm not > sure what could happen in the compiler (implicit cast, sign extension...) > Yea, converting to uint32_t is the way to go. > > > + && 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. > > [..] > > > +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. > > > [..] > > > + if ( !violation ) > > + return true; > > + > > + /* First, handle rx2rw and n2rwx conversion automatically. */ > > + if ( npfec.write_access && xma == XENMEM_access_rx2rw ) > > + { > > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1, > > + 0, ~0, XENMEM_access_rw); > > + return false; > > + } > > + else if ( xma == XENMEM_access_n2rwx ) > > + { > > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1, > > + 0, ~0, XENMEM_access_rwx); > > + } > > + > > + /* Otherwise, check if there is a memory event listener, and send > the message along */ > > + if ( !mem_event_check_ring( &v->domain->mem_event->access ) ) > > NIT: if ( !mem_event_check_ring(&v->domain->mem_event->access) ) > Ack. > > > + { > > + /* No listener */ > > + if ( p2m->access_required ) > > + { > > + gdprintk(XENLOG_INFO, "Memory access permissions failure, " > > + "no mem_event listener VCPU %d, dom > %d\n", > > + v->vcpu_id, v->domain->domain_id); > > + domain_crash(v->domain); > > + } > > + else > > + { > > + /* n2rwx was already handled */ > > + if ( xma != XENMEM_access_n2rwx) > > NIT: if ( ... ) > Ack. > > [..] > > > +/* Set access type for a region of pfns. > > + * If start_pfn == -1ul, sets the default access type */ > > +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t > nr, > > + uint32_t start, uint32_t mask, xenmem_access_t > access) > > +{ > > [..] > > > + rc = apply_p2m_changes(d, MEMACCESS, > > + pfn_to_paddr(pfn+start), > pfn_to_paddr(pfn+nr), > > + 0, MATTR_MEM, mask, 0, a); > > + > > + if ( rc < 0 ) > > + return rc; > > + else if ( rc > 0 ) > > + return start+rc; > > start + rc > Ack. > > > + > > + flush_tlb_domain(d); > > NIT: Missing blank line. > > Ack. > Regards, > > > -- > Julien Grall > > Thanks! Tamas --001a113a96ca3b5cc30503d22839 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


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

On 09/23/2014 02:14 PM, Tamas K Lengyel wrote:
> This patch enables to store, set, check and deliver LPAE R/W mem_event= s.
> As the LPAE PTE's lack enough available software programmable bits= ,
> we store the permissions in a Radix tree. A custom boolean, access_in_= use,
> specifies if the tree is in use to avoid uneccessary lookups on an emp= ty tree.

unecessary

Ack.
= =A0
[..]

> +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned= long pfn,

Shouldn't "int" enough for the return type?

> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0p2m_access_t a)
> +{
> +=A0 =A0 long rc;

NIT: missing new line here.

Ack.=
=A0

[..]

>=A0 /* Put any references on the single 4K page referenced by pte.=A0 T= ODO:
> @@ -553,13 +584,22 @@ static int apply_one_level(struct domain *d,
>=A0 =A0 =A0 =A0 =A0 if ( p2m_valid(orig_pte) )
>=A0 =A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_DESCEND;
>
> -=A0 =A0 =A0 =A0 if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_s= ize) )
> +=A0 =A0 =A0 =A0 if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_s= ize) &&
> +=A0 =A0 =A0 =A0 =A0 =A0/* We only create superpages when mem_access i= s not in use. */
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0(level =3D=3D 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.
=A0

[..]

> +=A0 =A0 case MEMACCESS:
> +=A0 =A0 =A0 =A0 if ( level < 3 )
> +=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 *addr +=3D level_size;
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_PROGRESS_NOP;
> +=A0 =A0 =A0 =A0 =A0 =A0 }
> +
> +=A0 =A0 =A0 =A0 =A0 =A0 /* Shatter large pages as we descend */
> +=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 rc =3D p2m_shatter_page(d, entry, lev= el, flush_cache);
> +
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( rc < 0 )
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
> +=A0 =A0 =A0 =A0 =A0 =A0 } /* else: an existing table mapping -> de= scend */
> +
> +=A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_DESCEND;
> +=A0 =A0 =A0 =A0 }
> +=A0 =A0 =A0 =A0 else
> +=A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 pte =3D orig_pte;
> +
> +=A0 =A0 =A0 =A0 =A0 =A0 if ( !p2m_table(pte) )
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pte.bits =3D 0;
> +
> +=A0 =A0 =A0 =A0 =A0 =A0 if ( p2m_valid(pte) )
> +=A0 =A0 =A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ASSERT(pte.p2m.type !=3D 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 cop= ied from p2m_lookup. Can it even happen that something passes p2m_valid() b= ut have a type of p2m_invalid? I think that just signals that something is = very wrong.
=A0

[..]

> @@ -821,6 +912,21 @@ static int apply_p2m_changes(struct domain *d, >=A0 =A0 =A0 =A0 =A0 =A0 =A0 count =3D 0;
>=A0 =A0 =A0 =A0 =A0 }
>
> +=A0 =A0 =A0 =A0 /*
> +=A0 =A0 =A0 =A0 =A0* Preempt setting mem_access permissions as requir= ed by XSA-89,
> +=A0 =A0 =A0 =A0 =A0* if it's not the last iteration.
> +=A0 =A0 =A0 =A0 =A0*/
> +=A0 =A0 =A0 =A0 if ( op =3D=3D MEMACCESS && count )
> +=A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 int progress =3D paddr_to_pfn(addr) - start_g= pfn + 1;

uint32_t?

Ack.
=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">

NIT: Missing blank line.

> +=A0 =A0 =A0 =A0 =A0 =A0 if ( (end_gpfn-start_gpfn) > progress &= ;& !(progress & mask)

NIT: (end_gpfn - start_gpfn)

Also you are comparing with an "int" with an "unsigned long&= quot;. I'm not
sure what could happen in the compiler (implicit cast, sign extension...)

Yea, converting to uint32_t is the way t= o go.
=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 rc =3D progress;
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 =3D true). Does memaccess does a TLB flush<= br> somewhere else?

Yes, at the end of p2m_= set_mem_access once all PTEs are updated successfully. I guess we could flu= sh the TLB as we are progressing as well, it wouldn't hurt.
=A0

[..]

> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct np= fec npfec)
> +{
> +=A0 =A0 int rc;
> +=A0 =A0 bool_t violation;
> +=A0 =A0 xenmem_access_t xma;
> +=A0 =A0 mem_event_request_t *req;
> +=A0 =A0 struct vcpu *v =3D current;
> +=A0 =A0 struct p2m_domain *p2m =3D p2m_get_hostp2m(v->domain);
> +
> +=A0 =A0 /* Mem_access is not in use. */
> +=A0 =A0 if ( !p2m->access_in_use )
> +=A0 =A0 =A0 =A0 return true;

AFAIU, it's not possible to call this function when mem access i= s 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 an= d it is called every time there is a permission fault in the second stage t= ranslation. This check here just makes sure the function returns as fast as= possible when not in use.
=A0


[..]

> +=A0 =A0 if ( !violation )
> +=A0 =A0 =A0 =A0 return true;
> +
> +=A0 =A0 /* First, handle rx2rw and n2rwx conversion automatically. */=
> +=A0 =A0 if ( npfec.write_access && xma =3D=3D XENMEM_access_r= x2rw )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 rc =3D p2m_set_mem_access(v->domain, paddr_to_pfn(= gpa), 1,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 0, ~0= , XENMEM_access_rw);
> +=A0 =A0 =A0 =A0 return false;
> +=A0 =A0 }
> +=A0 =A0 else if ( xma =3D=3D XENMEM_access_n2rwx )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 rc =3D p2m_set_mem_access(v->domain, paddr_to_pfn(= gpa), 1,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 0, ~0= , XENMEM_access_rwx);
> +=A0 =A0 }
> +
> +=A0 =A0 /* Otherwise, check if there is a memory event listener, and = send the message along */
> +=A0 =A0 if ( !mem_event_check_ring( &v->domain->mem_event-&= gt;access ) )

NIT: if ( !mem_event_check_ring(&v->domain->mem_event->= access) )

Ack.
=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 /* No listener */
> +=A0 =A0 =A0 =A0 if ( p2m->access_required )
> +=A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 gdprintk(XENLOG_INFO, "Memory access per= missions failure, "
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &= quot;no mem_event listener VCPU %d, dom %d\n",
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 v= ->vcpu_id, v->domain->domain_id);
> +=A0 =A0 =A0 =A0 =A0 =A0 domain_crash(v->domain);
> +=A0 =A0 =A0 =A0 }
> +=A0 =A0 =A0 =A0 else
> +=A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 /* n2rwx was already handled */
> +=A0 =A0 =A0 =A0 =A0 =A0 if ( xma !=3D XENMEM_access_n2rwx)

NIT: if ( ... )

Ack.
=A0
<= /div>

[..]

> +/* Set access type for a region of pfns.
> + * If start_pfn =3D=3D -1ul, sets the default access type */
> +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t= nr,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 uint32_t start, uint3= 2_t mask, xenmem_access_t access)
> +{

[..]

> +=A0 =A0 rc =3D apply_p2m_changes(d, MEMACCESS,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pfn_to_paddr(p= fn+start), pfn_to_paddr(pfn+nr),
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00, MATTR_MEM, = mask, 0, a);
> +
> +=A0 =A0 if ( rc < 0 )
> +=A0 =A0 =A0 =A0 return rc;
> +=A0 =A0 else if ( rc > 0 )
> +=A0 =A0 =A0 =A0 return start+rc;

start + rc

Ack.
=A0
=

> +
> +=A0 =A0 flush_tlb_domain(d);

NIT: Missing blank line.


Ack.
=A0
Regards,


--
Julien Grall


Thanks!
Tamas

--001a113a96ca3b5cc30503d22839-- --===============4629193160327628519== 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 --===============4629193160327628519==--