From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v5 12/17] xen/arm: Data abort exception (R/W) mem_events. Date: Fri, 12 Sep 2014 10:46:26 +0200 Message-ID: References: <1410355726-5599-1-git-send-email-tklengyel@sec.in.tum.de> <1410355726-5599-13-git-send-email-tklengyel@sec.in.tum.de> <541211F3.4050103@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8411057189950284043==" Return-path: In-Reply-To: <541211F3.4050103@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 --===============8411057189950284043== Content-Type: multipart/alternative; boundary=001a11c303080f039f0502da5237 --001a11c303080f039f0502da5237 Content-Type: text/plain; charset=ISO-8859-1 On Thu, Sep 11, 2014 at 11:19 PM, Julien Grall wrote: > Hello Tamas, > > You forgot to handle add the permission in the radix when the a table is > shattered. > > On 10/09/14 06:28, Tamas K Lengyel wrote: > >> #define P2M_ONE_DESCEND 0 >> #define P2M_ONE_PROGRESS_NOP 0x1 >> #define P2M_ONE_PROGRESS 0x10 >> @@ -504,6 +570,10 @@ static int apply_one_level(struct domain *d, >> page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0); >> if ( page ) >> { >> + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), >> a); >> > > It's possible to allocate a 2M/1G mapping here. In the case of memaccess > you only want 4K mapping for granularity. > Right, I should only set it if level==3. > > + if ( rc < 0 ) >> > > You should free the page via free_domheap_pages if Xen fail to adds the > access type in the radix tree. > Ack. > + return rc; >> + >> pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a); >> if ( level < 3 ) >> pte.p2m.table = 0; >> @@ -538,6 +608,10 @@ static int apply_one_level(struct domain *d, >> /* We do not handle replacing an existing table with a >> superpage */ >> (level == 3 || !p2m_table(orig_pte)) ) >> { >> + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a); >> + if ( rc < 0 ) >> + return rc; >> + >> > > Same remark here about the mapping. > > Ack. > /* New mapping is superpage aligned, make it */ >> pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a); >> if ( level < 3 ) >> @@ -663,6 +737,7 @@ static int apply_one_level(struct domain *d, >> >> memset(&pte, 0x00, sizeof(pte)); >> p2m_write_pte(entry, pte, flush_cache); >> + radix_tree_delete(&p2m->mem_access_settings, >> paddr_to_pfn(*addr)); >> >> *addr += level_size; >> *maddr += level_size; >> @@ -707,6 +782,53 @@ static int apply_one_level(struct domain *d, >> *addr += PAGE_SIZE; >> return P2M_ONE_PROGRESS_NOP; >> } >> + >> + case MEMACCESS: >> + if ( level < 3 ) >> + { >> + if ( !p2m_valid(orig_pte) ) >> + { >> + (*addr)++; >> > > Why increment by 1? You the PTE doesn't contain valid mapping you want to > skip the whole level range. ie: > > *addr += level_size; > > It doesn't make a difference, apply_p2m_changes is called with start=paddr, end=paddr+1 from a separate loop. So just incrementing it by one or a whole level achieves the same effect, that is, the apply_p2m_changes loop breaks. > + return P2M_ONE_PROGRESS_NOP; >> + } >> + >> + /* Shatter large pages as we descend */ >> + if ( p2m_mapping(orig_pte) ) >> + { >> + rc = p2m_create_table(d, entry, >> + level_shift - PAGE_SHIFT, >> flush_cache); >> + if ( rc < 0 ) >> + return rc; >> + >> + p2m->stats.shattered[level]++; >> + p2m->stats.mappings[level]--; >> + p2m->stats.mappings[level+1] += LPAE_ENTRIES; >> + } /* else: an existing table mapping -> descend */ >> + >> > > This piece of code is exactly the same in INSERT, REMOVE and now > MEMACCESS. I would create an helper to shatter and update the stats. > Ack. > > + 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); >> + >> + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), >> a); >> + if ( rc < 0 ) >> + return rc; >> + >> + p2m_set_permission(&pte, pte.p2m.type, a); >> + p2m_write_pte(entry, pte, flush_cache); >> + } >> + >> + (*addr)++; >> > > *addr += PAGE_SIZE; > > [..] > > > +/* 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; >> + paddr_t paddr; >> + >> + 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; >> + } >> + >> + for ( pfn += start; nr > start; ++pfn ) >> + { >> + paddr = pfn_to_paddr(pfn); >> + rc = apply_p2m_changes(d, MEMACCESS, paddr, paddr+1, 0, >> MATTR_MEM, 0, a); >> > > Hmmm... why didn't you call directly apply_p2m_changes with the whole > range? > > Because the hypercall continuation. Setting mem_access permissions needs to be preemptible and it has its own separate routine to do that here. See http://xenbits.xen.org/xsa/advisory-89.html for more info. > [..] > > > +int p2m_get_mem_access(struct domain *d, unsigned long gpfn, >> + xenmem_access_t *access) >> +{ >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + void *i; >> + unsigned 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 ) >> + return -ESRCH; >> > > If the gpfn is not in the radix tree, it means that either the mapping > doesn't exists or the access type is p2m_access_rwx. > > You handle the former case but not the latter. > Ack. > > Regards, > > -- > Julien Grall > > Thanks! Tamas --001a11c303080f039f0502da5237 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Thu, Sep 11, 2014 at 11:19 PM, Julien Grall <julien.grall@lin= aro.org> wrote:
Hello Tamas,

You forgot to handle add the permission in the radix when the a table is sh= attered.

On 10/09/14 06:28, Tamas K Lengyel wrote:
=A0 #define P2M_ONE_DESCEND=A0 =A0 =A0 =A0 0
=A0 #define P2M_ONE_PROGRESS_NOP=A0 =A00x1
=A0 #define P2M_ONE_PROGRESS=A0 =A0 =A0 =A00x10
@@ -504,6 +570,10 @@ static int apply_one_level(struct domain *d,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 page =3D alloc_domheap_pages(d, level_shift - P= AGE_SHIFT, 0);
=A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( page )
=A0 =A0 =A0 =A0 =A0 =A0 =A0 {
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_mem_access_radix_set(p2m, paddr= _to_pfn(*addr), a);

 It's possible to allocate a 2M/1G mapping here. In the case of memacces= s you only want 4K mapping for granularity.

Right, I should only set it if level=3D=3D3. <= br>
=A0

+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( rc < 0 )

You should free the page via free_domheap_pages if Xen fail to adds the acc= ess type in the radix tree.
=A0

Ack.
=A0
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
+
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pte =3D mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( level < 3 )
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pte.p2m.table =3D 0;
@@ -538,6 +608,10 @@ static int apply_one_level(struct domain *d,
=A0 =A0 =A0 =A0 =A0 =A0 =A0/* We do not handle replacing an existing table = with a superpage */
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(level =3D=3D 3 || !p2m_table(orig_pte)) ) =A0 =A0 =A0 =A0 =A0 {
+=A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_mem_access_radix_set(p2m, paddr_to_pfn(= *addr), a);
+=A0 =A0 =A0 =A0 =A0 =A0 if ( rc < 0 )
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
+

 Same remark here about the mapping.


Ack.
=A0
=A0 =A0 =A0 =A0 =A0 =A0 =A0 /* New mapping is superpage aligned, make it */=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 pte =3D mfn_to_p2m_entry(*maddr >> PAGE_S= HIFT, mattr, t, a);
=A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( level < 3 )
@@ -663,6 +737,7 @@ static int apply_one_level(struct domain *d,

=A0 =A0 =A0 =A0 =A0 memset(&pte, 0x00, sizeof(pte));
=A0 =A0 =A0 =A0 =A0 p2m_write_pte(entry, pte, flush_cache);
+=A0 =A0 =A0 =A0 radix_tree_delete(&p2m->mem_access_settings,= paddr_to_pfn(*addr));

=A0 =A0 =A0 =A0 =A0 *addr +=3D level_size;
=A0 =A0 =A0 =A0 =A0 *maddr +=3D level_size;
@@ -707,6 +782,53 @@ static int apply_one_level(struct domain *d,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 *addr +=3D PAGE_SIZE;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 return P2M_ONE_PROGRESS_NOP;
=A0 =A0 =A0 =A0 =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)++;

 Why increment by 1? You the PTE doesn't contain valid mapping you want = to skip the whole level range. ie:

*addr +=3D level_size;


It doesn't make a differenc= e, apply_p2m_changes is called with start=3Dpaddr, end=3Dpaddr+1 from a sep= arate loop. So just incrementing it by one or a whole level achieves the sa= me effect, that is, the apply_p2m_changes loop breaks.
=A0
+=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_create_table(d, entry,
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 level_shift - PAGE_SHIFT, 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 =A0 =A0 p2m->stats.shattered[level]++;
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 p2m->stats.mappings[level]--;
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 p2m->stats.mappings[level+1] +=3D LPAE_= ENTRIES;
+=A0 =A0 =A0 =A0 =A0 =A0 } /* else: an existing table mapping -> descend= */
+

 This piece of code is exactly the same in INSERT, REMOVE and now MEMACCESS.= I would create an helper to shatter and update the stats.=

Ack.
=A0

+=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);
+
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m_mem_access_radix_set(p2m, paddr= _to_pfn(*addr), a);
+=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 =A0 =A0 p2m_set_permission(&pte, pte.p2m.type,= a);
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 p2m_write_pte(entry, pte, flush_cache); +=A0 =A0 =A0 =A0 =A0 =A0 }
+
+=A0 =A0 =A0 =A0 =A0 =A0 (*addr)++;

 *addr +=3D PAGE_SIZE;

[..]


+/* 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 paddr_t paddr;
+
+=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 for ( pfn +=3D start; nr > start; ++pfn )
+=A0 =A0 {
+=A0 =A0 =A0 =A0 paddr =3D pfn_to_paddr(pfn);
+=A0 =A0 =A0 =A0 rc =3D apply_p2m_changes(d, MEMACCESS, paddr, paddr+1, 0, = MATTR_MEM, 0, a);

Hmmm... why didn't you call directly apply_p2m_changes with the whole r= ange?


Because the hypercall continuation. Se= tting mem_access permissions needs to be preemptible and it has its own sep= arate routine to do that here. See http://xenbits.xen.org/xsa/advisory-89.html for more in= fo.
=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) +{
+=A0 =A0 struct p2m_domain *p2m =3D p2m_get_hostp2m(d);
+=A0 =A0 void *i;
+=A0 =A0 unsigned 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 )
+=A0 =A0 =A0 =A0 return -ESRCH;

If the gpfn is not in the radix tree, it means that either the mapping does= n't exists or the access type is p2m_access_rwx.

You handle the former case but not the latter.

Ack.
=A0

Regards,

--
Julien Grall


Thanks!
Tamas
--001a11c303080f039f0502da5237-- --===============8411057189950284043== 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 --===============8411057189950284043==--