From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v3 10/15] xen/arm: Data abort exception (R/W) mem_events. Date: Tue, 2 Sep 2014 11:06:22 +0200 Message-ID: References: <1409581329-2607-1-git-send-email-tklengyel@sec.in.tum.de> <1409581329-2607-11-git-send-email-tklengyel@sec.in.tum.de> <5404E02E.7010406@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4435967785454887389==" Return-path: In-Reply-To: <5404E02E.7010406@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 --===============4435967785454887389== Content-Type: multipart/alternative; boundary=001a11c29feef662640502116edd --001a11c29feef662640502116edd Content-Type: text/plain; charset=ISO-8859-1 On Mon, Sep 1, 2014 at 11:07 PM, Julien Grall wrote: > Hello Tamas, > > > On 01/09/14 10:22, Tamas K Lengyel wrote: > >> This patch enables to store, set, check and deliver LPAE R/W mem_events. >> > > I would expand a bit more the commit message to explain the logic of mem > event in ARM. > > I know you already explain it in patch #8 ("p2m type definitions and > changes") but I think it's more relevant to explain where the "real" code > is. Ack. > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index a6dea5b..e8f5671 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -10,6 +10,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -148,6 +149,74 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, >> paddr_t addr) >> return __map_domain_page(page); >> } >> >> +static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) >> +{ >> + /* First apply type permissions */ >> + switch (t) >> > > switch ( t ) > > Ack. > [..] > > > + /* Then restrict with access permissions */ >> + switch(a) >> > > switch ( a ) > > [..] > > Ack. > > /* >> * Lookup the MFN corresponding to a domain's PFN. >> * >> @@ -228,7 +297,7 @@ int p2m_pod_decrease_reservation(struct domain *d, >> } >> >> static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, >> - p2m_type_t t) >> + p2m_type_t t, p2m_access_t a) >> > > It looks strange to modify nearly all prototypes except this one in #6. > > Ack. > [..] > > > @@ -346,7 +385,7 @@ static int p2m_create_table(struct domain *d, lpae_t >> *entry, >> for ( i=0 ; i < LPAE_ENTRIES; i++ ) >> { >> pte = mfn_to_p2m_entry(base_pfn + >> (i<<(level_shift-LPAE_SHIFT)), >> - MATTR_MEM, t); >> + MATTR_MEM, t, p2m->default_access); >> > > I'm not familiar with xen memaccess. Do we need to track access to > intermediate page table (i.e Level 1 & 2)? > > Yes, we need to track if a violation happened as a result of mem-access restricting the pte permission or otherwise. From that perspective there is no difference if the violation happened on a large or 4k page. From a tracing perspective it makes more sense to have more granularity (4k pages only) so I do shatter the large pages when setting mem-access permissions. > > /* >> * First and second level super pages set p2m.table = 0, >> but >> @@ -366,7 +405,7 @@ static int p2m_create_table(struct domain *d, lpae_t >> *entry, >> >> unmap_domain_page(p); >> >> - pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid); >> + pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid, >> p2m->default_access); >> > > Same question here. > > [..] > > > +int p2m_mem_access_check(paddr_t gpa, vaddr_t gla, struct npfec npfec) >> > > This function only return 0/1, I would use bool_t here. > > To reply on your answer on the last version, this function is not used in > common code and x86 also use bool_t as return type. > > You are right, I confused it with p2m_set_mem_access. > > +{ >> + struct vcpu *v = current; >> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); >> + mem_event_request_t *req = NULL; >> + xenmem_access_t xma; >> + bool_t violation; >> + int rc; >> + >> + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma); >> + if ( rc ) >> + { >> + /* No setting was found, reinject */ >> + return 1; >> + } >> + else >> + { >> + /* 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); >> + ASSERT(rc == 0); >> > > It's not a good idea the ASSERT here. The function call radix_tree_insert > in p2m_set_mem_access may fail because there is a memory allocation via > xmalloc inside. > > I suspect x86 adds the ASSERT because the mapping always exists and there > is no memory allocation in set_entry. I will let x86 folks confirm or not > my purpose. > > We can certainly adjust it here if required, this is mainly just copy-paste from x86. > > + return 0; >> + } >> + else if ( xma == XENMEM_access_n2rwx ) >> + { >> + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1, >> + 0, ~0, XENMEM_access_rwx); >> + ASSERT(rc == 0); >> > > Same remark here. > > Ack. > > + } >> + } >> + >> + /* Otherwise, check if there is a memory event listener, and send >> the message along */ >> + if ( !mem_event_check_ring( &v->domain->mem_event->access ) ) >> + { >> + /* 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) >> + { >> + /* A listener is not required, so clear the access >> + * restrictions. */ >> + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1, >> + 0, ~0, XENMEM_access_rwx); >> + ASSERT(rc == 0); >> > > Same here. > > Ack. > > +void p2m_mem_access_resume(struct domain *d) >> +{ >> + mem_event_response_t rsp; >> + >> + /* Pull all responses off the ring */ >> + while( mem_event_get_response(d, &d->mem_event->access, &rsp) ) >> + { >> + struct vcpu *v; >> + >> + if ( rsp.flags & MEM_EVENT_FLAG_DUMMY ) >> + continue; >> + >> + /* Validate the vcpu_id in the response. */ >> + if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] ) >> + continue; >> + >> + v = d->vcpu[rsp.vcpu_id]; >> + >> + /* Unpause domain */ >> + if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED ) >> + mem_event_vcpu_unpause(v); >> + } >> +} >> > > This function looks very similar, if not a copy, of the x86 one. Can't we > share the code? > > That would make sense, abstract it out into common mem_access code. > > +/* 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) >> +{ >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + p2m_access_t a; >> + long rc = 0; >> + >> + static const p2m_access_t memaccess[] = { >> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac >> + ACCESS(n), >> + ACCESS(r), >> + ACCESS(w), >> + ACCESS(rw), >> + ACCESS(x), >> + ACCESS(rx), >> + ACCESS(wx), >> + ACCESS(rwx), >> +#undef ACCESS >> + }; >> + >> + switch ( access ) >> + { >> + case 0 ... ARRAY_SIZE(memaccess) - 1: >> + a = memaccess[access]; >> + break; >> + case XENMEM_access_default: >> + a = p2m->default_access; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + /* If request to set default access */ >> + if ( pfn == ~0ul ) >> + { >> + p2m->default_access = a; >> + return 0; >> + } >> + >> + spin_lock(&p2m->lock); >> + for ( pfn += start; nr > start; ++pfn ) >> > > Why don't you reuse apply_p2m_changes? everything to get/update a pte is > there and it contains few optimization. > > Also this would avoid to duplicate the shatter code in p2m_set_entry. Honestly, I tried to wrap my head around apply_p2m_changes and it is already quite complex. While I see I could apply the type/access permissions with it over a range, I'm not entirely sure how I would make it force-shatter all large pages. It was just easier (for now) to do it manually. > > > + { >> + >> + bool_t pte_update = p2m_set_entry(d, pfn_to_paddr(pfn), a); >> + >> + if ( !pte_update ) >> + break; >> > > Shouldn't you continue here? The other pages in the batch may require > updates. > > This is the same approach as in x86. > [..] > > > +int p2m_get_mem_access(struct domain *d, unsigned long gpfn, >> + xenmem_access_t *access) >> +{ >> > > I think this function is not complete. You only set the variable access > when the page frame number has been found in the radix tree. But the page > may use the default access which could result to a trap in Xen. > > On x86, We agree that the default access value is stored in the entry. So > if the default access value changes, Xen will retrieved the value > previously stored in the pte. > > With your current solution, reproduce this behavior on ARM will be > difficult, unless you add every page in the radix tree. > > But I don't have better idea for now. Right, I missed page additions with default_access. With so few software programmable bits available in the PTE, we have no other choice but to store the permission settings separately. I just need to make sure that the radix tree is updated when a pte is added/removed. > > > + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + void *i; >> + int index; >> + >> + static const xenmem_access_t memaccess[] = { >> +#define ACCESS(ac) [XENMEM_access_##ac] = XENMEM_access_##ac >> + ACCESS(n), >> + ACCESS(r), >> + ACCESS(w), >> + ACCESS(rw), >> + ACCESS(x), >> + ACCESS(rx), >> + ACCESS(wx), >> + ACCESS(rwx), >> +#undef ACCESS >> + }; >> + >> + /* If request to get default access */ >> + if ( gpfn == ~0ull ) >> + { >> + *access = memaccess[p2m->default_access]; >> + return 0; >> + } >> + >> + spin_lock(&p2m->lock); >> + >> + i = radix_tree_lookup(&p2m->mem_access_settings, gpfn); >> + >> + spin_unlock(&p2m->lock); >> + >> + if (!i) >> > > if ( !i ) > > Ack. > > + return -ESRCH; >> + >> + index = radix_tree_ptr_to_int(i); >> + >> + if ( (unsigned) index >= ARRAY_SIZE(memaccess) ) >> + return -ERANGE; >> > > You are casting to unsigned all usage of index within this function. Why > not directly define index as an "unsigned int"? > > The radix tree returns an int but I guess could do that. > > + >> + *access = memaccess[ (unsigned) index]; >> > > memaccess[(unsigned)index]; > > > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/ >> processor.h >> index 0cc5b6d..b844f1d 100644 >> --- a/xen/include/asm-arm/processor.h >> +++ b/xen/include/asm-arm/processor.h >> @@ -262,6 +262,36 @@ enum dabt_size { >> DABT_DOUBLE_WORD = 3, >> }; >> >> +/* Data abort data fetch status codes */ >> +enum dabt_dfsc { >> + DABT_DFSC_ADDR_SIZE_0 = 0b000000, >> + DABT_DFSC_ADDR_SIZE_1 = 0b000001, >> + DABT_DFSC_ADDR_SIZE_2 = 0b000010, >> + DABT_DFSC_ADDR_SIZE_3 = 0b000011, >> + DABT_DFSC_TRANSLATION_0 = 0b000100, >> + DABT_DFSC_TRANSLATION_1 = 0b000101, >> + DABT_DFSC_TRANSLATION_2 = 0b000110, >> + DABT_DFSC_TRANSLATION_3 = 0b000111, >> + DABT_DFSC_ACCESS_1 = 0b001001, >> + DABT_DFSC_ACCESS_2 = 0b001010, >> + DABT_DFSC_ACCESS_3 = 0b001011, >> + DABT_DFSC_PERMISSION_1 = 0b001101, >> + DABT_DFSC_PERMISSION_2 = 0b001110, >> + DABT_DFSC_PERMISSION_3 = 0b001111, >> + DABT_DFSC_SYNC_EXT = 0b010000, >> + DABT_DFSC_SYNC_PARITY = 0b011000, >> + DABT_DFSC_SYNC_EXT_TTW_0 = 0b010100, >> + DABT_DFSC_SYNC_EXT_TTW_1 = 0b010101, >> + DABT_DFSC_SYNC_EXT_TTW_2 = 0b010110, >> + DABT_DFSC_SYNC_EXT_TTW_3 = 0b010111, >> + DABT_DFSC_SYNC_PARITY_TTW_0 = 0b011100, >> + DABT_DFSC_SYNC_PARITY_TTW_1 = 0b011101, >> + DABT_DFSC_SYNC_PARITY_TTW_2 = 0b011110, >> + DABT_DFSC_SYNC_PARITY_TTW_3 = 0b011111, >> + DABT_DFSC_ALIGNMENT = 0b100001, >> + DABT_DFSC_TLB_CONFLICT = 0b110000, >> +}; >> + >> > > I'm not sure if it's necessary to define every possible value. > > I figured for the sake of completeness it would make sense. I really only use the PERMISSION values so the rest could be removed. Thanks, Tamas > Regards, > > -- > Julien Grall > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > --001a11c29feef662640502116edd Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable



On Mon, Sep 1, 2014 at 11:07 PM, Julien Grall <= ;julien.grall@= linaro.org> wrote:
Hello Tamas,


On 01/09/14 10:22, Tamas K Lengyel wrote:
This patch enables to store, set, check and deliver LPAE R/W mem_events.

I would expand a bit more the commit message to explain the logic of mem ev= ent in ARM.

I know you already explain it in patch #8 ("p2m type definitions and c= hanges") but I think it's more relevant to explain where the "= ;real" code is.

Ack.
=A0


diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a6dea5b..e8f5671 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -10,6 +10,7 @@
=A0 #include <asm/event.h>
=A0 #include <asm/hardirq.h>
=A0 #include <asm/page.h>
+#include <xen/radix-tree.h>
=A0 #include <xen/mem_event.h>
=A0 #include <public/mem_event.h>
=A0 #include <xen/mem_access.h>
@@ -148,6 +149,74 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, p= addr_t addr)
=A0 =A0 =A0 return __map_domain_page(page);
=A0 }

+static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) +{
+=A0 =A0 /* First apply type permissions */
+=A0 =A0 switch (t)

switch ( t )


Ack.
=A0
[..]


+=A0 =A0 /* Then restrict with access permissions */
+=A0 =A0 switch(a)

switch ( a )

[..]

Ack.
=A0

=A0 /*
=A0 =A0* Lookup the MFN corresponding to a domain's PFN.
=A0 =A0*
@@ -228,7 +297,7 @@ int p2m_pod_decrease_reservation(struct domain *= d,
=A0 }

=A0 static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, -=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0p2m_type_t = t)
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0p2m_type_t = t, p2m_access_t a)

It looks strange to modify nearly all prototypes except this one in #6.


Ack.
=A0
[..]


@@ -346,7 +385,7 @@ static int p2m_create_table(struct domain *d, lpae_t *e= ntry,
=A0 =A0 =A0 =A0 =A0 =A0for ( i=3D0 ; i < LPAE_ENTRIES; i++ )
=A0 =A0 =A0 =A0 =A0 =A0{
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pte =3D mfn_to_p2m_entry(base_pfn + (i<&l= t;(level_shift-LPAE_SHIFT)),
-=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 MA= TTR_MEM, t);
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 MA= TTR_MEM, t, p2m->default_access);

I'm not familiar with xen memaccess. Do we need to track access to inte= rmediate page table (i.e Level 1 & 2)?


Yes, we need to track if a violation happened as = a result of mem-access restricting the pte permission or otherwise. From th= at perspective there is no difference if the violation happened on a large = or 4k page. From a tracing perspective it makes more sense to have more gra= nularity (4k pages only) so I do shatter the large pages when setting mem-a= ccess permissions.
=A0

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * First and second level super pages set p2= m.table =3D 0, but
@@ -366,7 +405,7 @@ static int p2m_create_table(struct domain *d, lpae_t *e= ntry,

=A0 =A0 =A0 unmap_domain_page(p);

-=A0 =A0 pte =3D mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_= invalid);
+=A0 =A0 pte =3D mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_= invalid, p2m->default_access);

Same question here.

[..]


+int p2m_mem_access_check(paddr_t gpa, vaddr_t gla, struct npfec npfec)

This function only return 0/1, I would use bool_t here.

To reply on your answer on the last version, this function is not used in c= ommon code and x86 also use bool_t as return type.


You are right, I confused it with p2m_set= _mem_access.
=A0

+{
+=A0 =A0 struct vcpu *v =3D current;
+=A0 =A0 struct p2m_domain *p2m =3D p2m_get_hostp2m(v->domain);
+=A0 =A0 mem_event_request_t *req =3D NULL;
+=A0 =A0 xenmem_access_t xma;
+=A0 =A0 bool_t violation;
+=A0 =A0 int rc;
+
+=A0 =A0 rc =3D p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xm= a);
+=A0 =A0 if ( rc )
+=A0 =A0 {
+=A0 =A0 =A0 =A0 /* No setting was found, reinject */
+=A0 =A0 =A0 =A0 return 1;
+=A0 =A0 }
+=A0 =A0 else
+=A0 =A0 {
+=A0 =A0 =A0 =A0 /* First, handle rx2rw and n2rwx conversion automatically.= */
+=A0 =A0 =A0 =A0 if ( npfec.write_access && xma =3D=3D XENMEM_acces= s_rx2rw )
+=A0 =A0 =A0 =A0 {
+=A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_set_mem_access(v->domain, paddr_to_p= fn(gpa), 1,
+=A0 =A0 =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 =A0 =A0 ASSERT(rc =3D=3D 0);

It's not a good idea the ASSERT here. The function call radix_tree_inse= rt in p2m_set_mem_access may fail because there is a memory allocation via = xmalloc inside.

I suspect x86 adds the ASSERT because the mapping always exists and there i= s no memory allocation in set_entry. I will let x86 folks confirm or not my= purpose.


We can = certainly adjust it here if required, this is=A0 mainly just copy-paste fro= m x86.
=A0

+=A0 =A0 =A0 =A0 =A0 =A0 return 0;
+=A0 =A0 =A0 =A0 }
+=A0 =A0 =A0 =A0 else if ( xma =3D=3D XENMEM_access_n2rwx )
+=A0 =A0 =A0 =A0 {
+=A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_set_mem_access(v->domain, paddr_to_p= fn(gpa), 1,
+=A0 =A0 =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 =A0 =A0 ASSERT(rc =3D=3D 0);

Same remark here.


Ack.
= =A0

+=A0 =A0 =A0 =A0 }
+=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->ac= cess ) )
+=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 permissi= ons failure, "
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "= 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)
+=A0 =A0 =A0 =A0 =A0 =A0 {
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* A listener is not required, so clear th= e access
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* restrictions. */
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_set_mem_access(v->domain, pa= ddr_to_pfn(gpa), 1,
+=A0 =A0 =A0 =A0 =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 =A0 =A0 =A0 =A0 ASSERT(rc =3D=3D 0);

Same here.


Ack.
=A0

+void p2m_mem_access_resume(struct domain *d)
+{
+=A0 =A0 mem_event_response_t rsp;
+
+=A0 =A0 /* Pull all responses off the ring */
+=A0 =A0 while( mem_event_get_response(d, &d->mem_event->access, = &rsp) )
+=A0 =A0 {
+=A0 =A0 =A0 =A0 struct vcpu *v;
+
+=A0 =A0 =A0 =A0 if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
+=A0 =A0 =A0 =A0 =A0 =A0 continue;
+
+=A0 =A0 =A0 =A0 /* Validate the vcpu_id in the response. */
+=A0 =A0 =A0 =A0 if ( (rsp.vcpu_id >=3D d->max_vcpus) || !d->vcpu[= rsp.vcpu_id] )
+=A0 =A0 =A0 =A0 =A0 =A0 continue;
+
+=A0 =A0 =A0 =A0 v =3D d->vcpu[rsp.vcpu_id];
+
+=A0 =A0 =A0 =A0 /* Unpause domain */
+=A0 =A0 =A0 =A0 if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
+=A0 =A0 =A0 =A0 =A0 =A0 mem_event_vcpu_unpause(v);
+=A0 =A0 }
+}

This function looks very similar, if not a copy, of the x86 one. Can't = we share the code?

=
That would make sense, abstract it out into common mem_acces= s code.
=A0
=

+/* 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,<= br> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 uint32_t start, uint32_t m= ask, xenmem_access_t access)
+{
+=A0 =A0 struct p2m_domain *p2m =3D p2m_get_hostp2m(d);
+=A0 =A0 p2m_access_t a;
+=A0 =A0 long rc =3D 0;
+
+=A0 =A0 static const p2m_access_t memaccess[] =3D {
+#define ACCESS(ac) [XENMEM_access_##ac] =3D p2m_access_##ac
+=A0 =A0 =A0 =A0 ACCESS(n),
+=A0 =A0 =A0 =A0 ACCESS(r),
+=A0 =A0 =A0 =A0 ACCESS(w),
+=A0 =A0 =A0 =A0 ACCESS(rw),
+=A0 =A0 =A0 =A0 ACCESS(x),
+=A0 =A0 =A0 =A0 ACCESS(rx),
+=A0 =A0 =A0 =A0 ACCESS(wx),
+=A0 =A0 =A0 =A0 ACCESS(rwx),
+#undef ACCESS
+=A0 =A0 };
+
+=A0 =A0 switch ( access )
+=A0 =A0 {
+=A0 =A0 case 0 ... ARRAY_SIZE(memaccess) - 1:
+=A0 =A0 =A0 =A0 a =3D memaccess[access];
+=A0 =A0 =A0 =A0 break;
+=A0 =A0 case XENMEM_access_default:
+=A0 =A0 =A0 =A0 a =3D p2m->default_access;
+=A0 =A0 =A0 =A0 break;
+=A0 =A0 default:
+=A0 =A0 =A0 =A0 return -EINVAL;
+=A0 =A0 }
+
+=A0 =A0 /* If request to set default access */
+=A0 =A0 if ( pfn =3D=3D ~0ul )
+=A0 =A0 {
+=A0 =A0 =A0 =A0 p2m->default_access =3D a;
+=A0 =A0 =A0 =A0 return 0;
+=A0 =A0 }
+
+=A0 =A0 spin_lock(&p2m->lock);
+=A0 =A0 for ( pfn +=3D start; nr > start; ++pfn )

Why don't you reuse apply_p2m_changes? everything to get/update a pte i= s there and it contains few optimization.

Also this would avoid to duplicate the shatter code in p2m_set_entry.

Honestly, I tried to wrap my head around apply_p= 2m_changes and it is already quite complex. While I see I could apply the t= ype/access permissions with it over a range, I'm not entirely sure how = I would make it force-shatter all large pages. It was just easier (for now)= to do it manually.
=A0


+=A0 =A0 {
+
+=A0 =A0 =A0 =A0 bool_t pte_update =3D p2m_set_entry(d, pfn_to_paddr(pfn), = a);
+
+=A0 =A0 =A0 =A0 if ( !pte_update )
+=A0 =A0 =A0 =A0 =A0 =A0 break;

Shouldn't you continue here? The other pages in the batch may require u= pdates.


This is the same approach as in x86.
=A0
[..]


+int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0xenmem_access_t *access) +{

I think this function is not complete. You only set the variable access whe= n the page frame number has been found in the radix tree. But the page may = use the default access which could result to a trap in Xen.

On x86, We agree that the default access value is stored in the entry. So i= f the default access value changes, Xen will retrieved the value previously= stored in the pte.

With your current solution, reproduce this behavior on ARM will be difficul= t, unless you add every page in the radix tree.

But I don't have better idea for now.

R= ight, I missed page additions with default_access. With so few software pro= grammable bits available in the PTE, we have no other choice but to store t= he permission settings separately. I just need to make sure that the radix = tree is updated when a pte is added/removed.
=A0
=


+=A0 =A0 struct p2m_domain *p2m =3D p2m_get_hostp2m(d);
+=A0 =A0 void *i;
+=A0 =A0 int index;
+
+=A0 =A0 static const xenmem_access_t memaccess[] =3D {
+#define ACCESS(ac) [XENMEM_access_##ac] =3D XENMEM_access_##ac
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(n),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(r),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(w),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(rw),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(x),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(rx),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(wx),
+=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(rwx),
+#undef ACCESS
+=A0 =A0 };
+
+=A0 =A0 /* If request to get default access */
+=A0 =A0 if ( gpfn =3D=3D ~0ull )
+=A0 =A0 {
+=A0 =A0 =A0 =A0 *access =3D memaccess[p2m->default_access];
+=A0 =A0 =A0 =A0 return 0;
+=A0 =A0 }
+
+=A0 =A0 spin_lock(&p2m->lock);
+
+=A0 =A0 i =3D radix_tree_lookup(&p2m->mem_access_settings, g= pfn);
+
+=A0 =A0 spin_unlock(&p2m->lock);
+
+=A0 =A0 if (!i)

if ( !i )


Ack.
=A0

+=A0 =A0 =A0 =A0 return -ESRCH;
+
+=A0 =A0 index =3D radix_tree_ptr_to_int(i);
+
+=A0 =A0 if ( (unsigned) index >=3D ARRAY_SIZE(memaccess) )
+=A0 =A0 =A0 =A0 return -ERANGE;

You are casting to unsigned all usage of index within this function. Why no= t directly define index as an "unsigned int"?

=

The radix tree returns an int but I = guess could do that.
=A0

+
+=A0 =A0 *access =3D=A0 memaccess[ (unsigned) index];

memaccess[(unsigned)index];


diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/<= u>processor.h
index 0cc5b6d..b844f1d 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -262,6 +262,36 @@ enum dabt_size {
=A0 =A0 =A0 DABT_DOUBLE_WORD =3D 3,
=A0 };

+/* Data abort data fetch status codes */
+enum dabt_dfsc {
+=A0 =A0 DABT_DFSC_ADDR_SIZE_0=A0 =A0 =A0 =A0=3D 0b000000,
+=A0 =A0 DABT_DFSC_ADDR_SIZE_1=A0 =A0 =A0 =A0=3D 0b000001,
+=A0 =A0 DABT_DFSC_ADDR_SIZE_2=A0 =A0 =A0 =A0=3D 0b000010,
+=A0 =A0 DABT_DFSC_ADDR_SIZE_3=A0 =A0 =A0 =A0=3D 0b000011,
+=A0 =A0 DABT_DFSC_TRANSLATION_0=A0 =A0 =A0=3D 0b000100,
+=A0 =A0 DABT_DFSC_TRANSLATION_1=A0 =A0 =A0=3D 0b000101,
+=A0 =A0 DABT_DFSC_TRANSLATION_2=A0 =A0 =A0=3D 0b000110,
+=A0 =A0 DABT_DFSC_TRANSLATION_3=A0 =A0 =A0=3D 0b000111,
+=A0 =A0 DABT_DFSC_ACCESS_1=A0 =A0 =A0 =A0 =A0 =3D 0b001001,
+=A0 =A0 DABT_DFSC_ACCESS_2=A0 =A0 =A0 =A0 =A0 =3D 0b001010,
+=A0 =A0 DABT_DFSC_ACCESS_3=A0 =A0 =A0 =A0 =A0 =3D 0b001011,=A0 =A0
+=A0 =A0 DABT_DFSC_PERMISSION_1=A0 =A0 =A0 =3D 0b001101,
+=A0 =A0 DABT_DFSC_PERMISSION_2=A0 =A0 =A0 =3D 0b001110,
+=A0 =A0 DABT_DFSC_PERMISSION_3=A0 =A0 =A0 =3D 0b001111,
+=A0 =A0 DABT_DFSC_SYNC_EXT=A0 =A0 =A0 =A0 =A0 =3D 0b010000,
+=A0 =A0 DABT_DFSC_SYNC_PARITY=A0 =A0 =A0 =A0=3D 0b011000,
+=A0 =A0 DABT_DFSC_SYNC_EXT_TTW_0=A0 =A0 =3D 0b010100,
+=A0 =A0 DABT_DFSC_SYNC_EXT_TTW_1=A0 =A0 =3D 0b010101,
+=A0 =A0 DABT_DFSC_SYNC_EXT_TTW_2=A0 =A0 =3D 0b010110,
+=A0 =A0 DABT_DFSC_SYNC_EXT_TTW_3=A0 =A0 =3D 0b010111,
+=A0 =A0 DABT_DFSC_SYNC_PARITY_TTW_0 =3D 0b011100,
+=A0 =A0 DABT_DFSC_SYNC_PARITY_TTW_1 =3D 0b011101,
+=A0 =A0 DABT_DFSC_SYNC_PARITY_TTW_2 =3D 0b011110,
+=A0 =A0 DABT_DFSC_SYNC_PARITY_TTW_3 =3D 0b011111,
+=A0 =A0 DABT_DFSC_ALIGNMENT=A0 =A0 =A0 =A0 =A0=3D 0b100001,
+=A0 =A0 DABT_DFSC_TLB_CONFLICT=A0 =A0 =A0 =3D 0b110000,
+};
+

I'm not sure if it's necessary to define every possible value.


I figured for the sake of completeness= it would make sense. I really only use the PERMISSION values so the rest c= ould be removed.

Thanks,
Tamas
=A0
Regards,

--
Julien Grall

=A0

_______________________________________________
Xen-devel mailing list
Xen-devel@list= s.xen.org
http://lists.x= en.org/xen-devel

--001a11c29feef662640502116edd-- --===============4435967785454887389== 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 --===============4435967785454887389==--