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:17:14 +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> <1411570971.28127.63.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7352172064395635853==" Return-path: In-Reply-To: <1411570971.28127.63.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Tim Deegan , Julien Grall , 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 --===============7352172064395635853== Content-Type: multipart/alternative; boundary=001a11c1221254f6870503d204ff --001a11c1221254f6870503d204ff Content-Type: text/plain; charset=ISO-8859-1 On Wed, Sep 24, 2014 at 5:02 PM, Ian Campbell wrote: > On Tue, 2014-09-23 at 15:14 +0200, 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. > > "unnecessary". > Ack. > > > @@ -414,12 +417,40 @@ static int p2m_create_table(struct domain *d, > lpae_t *entry, > > return 0; > > } > > > > +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned > long pfn, > > + p2m_access_t a) > > +{ > > + long rc; > > + if ( p2m_access_rwx == a ) > > I think I mentioned this before, but please write your conditionals the > more conventional way around. > I guess my argument for this style was too much tin-foil-hat so Ack. > > > + { > > + if ( p2m->access_in_use ) > > + radix_tree_delete(&p2m->mem_access_settings, pfn); > > + > > + return 0; > > + } > > + > > + rc = radix_tree_insert(&p2m->mem_access_settings, pfn, > > + radix_tree_int_to_ptr(a)); > > + if ( -EEXIST == rc ) > > + { > > + /* If a setting existed already, change it to the new one */ > > + radix_tree_replace_slot( > > + radix_tree_lookup_slot( > > + &p2m->mem_access_settings, pfn), > > + radix_tree_int_to_ptr(a)); > > What a shame that the API doesn't allow you to do an insert or replace > in one go! > Could be extended I guess in the future. > > > @@ -591,9 +631,14 @@ static int apply_one_level(struct domain *d, > > > > case INSERT: > > if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) > && > > - /* We do not handle replacing an existing table with a > superpage */ > > - (level == 3 || !p2m_table(orig_pte)) ) > > + /* We do not handle replacing an existing table with a > superpage > > + * or when mem_access is in use. */ > > + (level == 3 || (!p2m_table(orig_pte) && > !p2m->access_in_use)) ) > > { > > + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a); > > + if ( rc < 0 ) > > + return rc; > > I expect the answer is yes, but just to check: It is desirable to be > able to insert with a specific non-defualt access perms, as opposed to > INSERT with defaults then MEMACCESS? > I would say yes for performance reasons. No need to go through the pages once for INSERT then once for MEMACCESS. > > > @@ -753,6 +799,49 @@ 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 += 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; > > I don't know if that matter for this specific sub op but what is > supposed to happen on error? Should partial work be undone? > IMHO if a page cannot be shattered something must be pretty screwed already (Xen running out of memory?) in which case un-doing the already shattered pages is the least of our concern.. > > > @@ -776,6 +865,8 @@ static int apply_p2m_changes(struct domain *d, > > unsigned int cur_root_table = ~0; > > unsigned int cur_offset[4] = { ~0, }; > > unsigned int count = 0; > > + unsigned long start_gpfn = paddr_to_pfn(start_gpaddr), > > + end_gpfn = paddr_to_pfn(end_gpaddr); > > You don't need to update end_gpaddr up, do you? > No, these two values are provided by the user and we only keep track of the progress made in updating the PTEs that fall in-range. > > > bool_t flush = false; > > bool_t flush_pt; > > > > @@ -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; > > + if ( (end_gpfn-start_gpfn) > progress && !(progress & mask) > > This differs from the x86 equivalent in that it is not "++progress". > Doesn't that mean that we can fail to make any progress at all? > It is technically ++progress, just the style is different. Note the +1 when declaring progress. Furthermore, the && count ensures that at least one iteration is always done when calling with MEMACCESS. > > > +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) [p2m_access_##ac] = XENMEM_access_##ac > > + ACCESS(n), > > + ACCESS(r), > > + ACCESS(w), > > + ACCESS(rw), > > + ACCESS(x), > > + ACCESS(rx), > > + ACCESS(wx), > > + ACCESS(rwx), > > + ACCESS(rx2rw), > > + ACCESS(n2rwx), > > +#undef ACCESS > > Unlike x86 I think you have pretty much complete freedom to define > p2m_access_*? Why not just make them 1:1? > > (one for a future cleanup if you prefer) > I moved p2m_access_t into common in this version of the series.. > > > + /* Seting was found in the Radix tree. */ > > "Setting" > Ack. Thanks, Tamas --001a11c1221254f6870503d204ff Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Wed, Sep 24, 2014 at 5:02 PM, Ian Campbell <Ian.Campbell@citr= ix.com> wrote:
On Tue, 2014-09-23 at 15:14 +0200, 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.

"unnecessary".

Ack.=A0

> @@ -414,12 +417,40 @@ static int p2m_create_table(struct domain *d, lp= ae_t *entry,
>=A0 =A0 =A0 return 0;
>=A0 }
>
> +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned= long pfn,
> +=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;
> +=A0 =A0 if ( p2m_access_rwx =3D=3D a )

I think I mentioned this before, but please write your conditionals = the
more conventional way around.

I guess m= y argument for this style was too much tin-foil-hat so Ack.
= =A0

> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 if ( p2m->access_in_use )
> +=A0 =A0 =A0 =A0 =A0 =A0 radix_tree_delete(&p2m->mem_access_set= tings, pfn);
> +
> +=A0 =A0 =A0 =A0 return 0;
> +=A0 =A0 }
> +
> +=A0 =A0 rc =3D radix_tree_insert(&p2m->mem_access_settings, pf= n,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0radix_tree_int= _to_ptr(a));
> +=A0 =A0 if ( -EEXIST =3D=3D rc )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 /* If a setting existed already, change it to the new= one */
> +=A0 =A0 =A0 =A0 radix_tree_replace_slot(
> +=A0 =A0 =A0 =A0 =A0 =A0 radix_tree_lookup_slot(
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &p2m->mem_access_settings, pfn= ),
> +=A0 =A0 =A0 =A0 =A0 =A0 radix_tree_int_to_ptr(a));

What a shame that the API doesn't allow you to do an insert or r= eplace
in one go!

Could be extended I guess in= the future.
=A0

> @@ -591,9 +631,14 @@ static int apply_one_level(struct domain *d,
>
>=A0 =A0 =A0 case INSERT:
>=A0 =A0 =A0 =A0 =A0 if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, = level_size) &&
> -=A0 =A0 =A0 =A0 =A0 =A0/* We do not handle replacing an existing tabl= e with a superpage */
> -=A0 =A0 =A0 =A0 =A0 =A0 =A0(level =3D=3D 3 || !p2m_table(orig_pte)) )=
> +=A0 =A0 =A0 =A0 =A0 =A0/* We do not handle replacing an existing tabl= e with a superpage
> +=A0 =A0 =A0 =A0 =A0 =A0 * or when mem_access is in use. */
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0(level =3D=3D 3 || (!p2m_table(orig_pte) &= amp;& !p2m->access_in_use)) )
>=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;

I expect the answer is yes, but just to check: It is desirable to be=
able to insert with a specific non-defualt access perms, as opposed to
INSERT with defaults then MEMACCESS?

I = would say yes for performance reasons. No need to go through the pages once= for INSERT then once for MEMACCESS.
=A0

> @@ -753,6 +799,49 @@ 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 +=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;

I don't know if that matter for this specific sub op but what is=
supposed to happen on error? Should partial work be undone?

IMHO if a page cannot be shattered something must be p= retty screwed already (Xen running out of memory?) in which case un-doing t= he already shattered pages is the least of our concern..
=A0<= /div>

> @@ -776,6 +865,8 @@ static int apply_p2m_changes(struct domain *d,
>=A0 =A0 =A0 unsigned int cur_root_table =3D ~0;
>=A0 =A0 =A0 unsigned int cur_offset[4] =3D { ~0, };
>=A0 =A0 =A0 unsigned int count =3D 0;
> +=A0 =A0 unsigned long start_gpfn =3D paddr_to_pfn(start_gpaddr),
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 end_gpfn =3D paddr_to_pfn(end_gpa= ddr);

You don't need to update end_gpaddr up, do you?
=

No, these two values are provided by the user and we on= ly keep track of the progress made in updating the PTEs that fall in-range.=
=A0

>=A0 =A0 =A0 bool_t flush =3D false;
>=A0 =A0 =A0 bool_t flush_pt;
>
> @@ -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;
> +=A0 =A0 =A0 =A0 =A0 =A0 if ( (end_gpfn-start_gpfn) > progress &= ;& !(progress & mask)

This differs from the x86 equivalent in that it is not "++progr= ess".
Doesn't that mean that we can fail to make any progress at all?

It is technically ++progress, just the style i= s different. Note the +1 when declaring progress. Furthermore, the &&am= p; count ensures that at least one iteration is always done when calling wi= th MEMACCESS.
=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 *acces= s)
> +{
> +=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) [p2m_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),
> +=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(rx2rw),
> +=A0 =A0 =A0 =A0 =A0 =A0 ACCESS(n2rwx),
> +#undef ACCESS

Unlike x86 I think you have pretty much complete freedom to define p2m_access_*? Why not just make them 1:1?

(one for a future cleanup if you prefer)

I moved p2m_access_t into common in this version of the series..
=A0

> +=A0 =A0 =A0 =A0 /* Seting was found in the Radix tree. */

"Setting"

Ack.

=
Thanks,
Tamas

--001a11c1221254f6870503d204ff-- --===============7352172064395635853== 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 --===============7352172064395635853==--