Re: [Xen-devel] [PATCH V4 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view

2019-12-18 Thread Alexandru Stefan ISAILA 


On 18.12.2019 12:45, Jan Beulich wrote:
> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct 
>> p2m_domain *p2m,
>>   return rc;
>>   }
>>   
>> -static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
>> -xenmem_access_t xaccess,
>> -p2m_access_t *paccess)
>> +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
>> + xenmem_access_t xaccess,
>> + p2m_access_t *paccess)
> 
> Would you mind taking the opportunity and add const to the first
> parameter?

Sure, given that there will be a new version, it will add it.

> 
>> @@ -2601,7 +2610,15 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t 
>> *idx)
>>   if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>>   continue;
>>   
>> -rc = p2m_activate_altp2m(d, i);
>> +p2m = d->arch.altp2m_p2m[i];
>> +
>> +if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, ) )
>> +{
>> +altp2m_list_unlock(d);
>> +return -EINVAL;
>> +}
> 
> Can this be pulled out of the locked region, ahead of the loop?
> The p2m getting passed in here (which is why it's in the loop)
> shouldn't have been in use yet, i.e. its ->default_access should
> have a known value. Hence this case could be taken care of
> independently, e.g. by adjusting xenmem_access_to_p2m_access()
> to cope with a NULL p2m coming in (producing whatever the default
> of the default is).
> 

Yes this sounds good. In xenmem_access_to_p2m_access() there can be a 
check like:

if ( !p2m )
 *paccess = p2m_access_rwx;
else
 *paccess = p2m->default_access;

But before I change this maybe Tamas or George have something to add?
And can this stay in the same patch or should it have a prereq one?

Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V4 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view

2019-12-18 Thread Jan Beulich 
On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct 
> p2m_domain *p2m,
>  return rc;
>  }
>  
> -static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
> -xenmem_access_t xaccess,
> -p2m_access_t *paccess)
> +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
> + xenmem_access_t xaccess,
> + p2m_access_t *paccess)

Would you mind taking the opportunity and add const to the first
parameter?

> @@ -2601,7 +2610,15 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t 
> *idx)
>  if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>  continue;
>  
> -rc = p2m_activate_altp2m(d, i);
> +p2m = d->arch.altp2m_p2m[i];
> +
> +if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, ) )
> +{
> +altp2m_list_unlock(d);
> +return -EINVAL;
> +}

Can this be pulled out of the locked region, ahead of the loop?
The p2m getting passed in here (which is why it's in the loop)
shouldn't have been in use yet, i.e. its ->default_access should
have a known value. Hence this case could be taken care of
independently, e.g. by adjusting xenmem_access_to_p2m_access()
to cope with a NULL p2m coming in (producing whatever the default
of the default is).

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH V4 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view

2019-12-17 Thread Alexandru Stefan ISAILA 
At this moment the default_access param from xc_altp2m_create_view is
not used.

This patch assigns default_access to p2m->default_access at the time of
initializing a new altp2m view.

Signed-off-by: Alexandru Isaila 
---
CC: Jan Beulich 
CC: Andrew Cooper 
CC: Wei Liu 
CC: "Roger Pau Monné" 
CC: George Dunlap 
CC: Ian Jackson 
CC: Julien Grall 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Razvan Cojocaru 
CC: Tamas K Lengyel 
CC: Petre Pircalabu 
CC: George Dunlap 
---
Changes since V3:
- Change type of hvmmem_default_access to xenmem_access_t
- Fix style issues
- Release lock before return.
---
 xen/arch/x86/hvm/hvm.c  |  3 ++-
 xen/arch/x86/mm/mem_access.c|  6 +++---
 xen/arch/x86/mm/p2m.c   | 27 ++-
 xen/include/asm-x86/p2m.h   |  3 ++-
 xen/include/public/hvm/hvm_op.h |  2 --
 xen/include/xen/mem_access.h|  4 
 6 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a129049d6b..d4b19d2412 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4687,7 +4687,8 @@ static int do_altp2m_op(
 }
 
 case HVMOP_altp2m_create_p2m:
-if ( !(rc = p2m_init_next_altp2m(d, )) )
+if ( !(rc = p2m_init_next_altp2m(d, ,
+ a.u.view.hvmmem_default_access)) )
 rc = __copy_to_guest(arg, , 1) ? -EFAULT : 0;
 break;
 
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 70f3528bb1..288c865ffa 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct 
p2m_domain *p2m,
 return rc;
 }
 
-static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
-xenmem_access_t xaccess,
-p2m_access_t *paccess)
+bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
+ xenmem_access_t xaccess,
+ p2m_access_t *paccess)
 {
 static const p2m_access_t memaccess[] = {
 #define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d381f6877f..d67326f8b7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -25,6 +25,7 @@
 
 #include  /* copy_from_guest() */
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2533,7 +2534,8 @@ void p2m_flush_altp2m(struct domain *d)
 altp2m_list_unlock(d);
 }
 
-static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
+static int p2m_activate_altp2m(struct domain *d, unsigned int idx,
+   p2m_access_t hvmmem_default_access)
 {
 struct p2m_domain *hostp2m, *p2m;
 int rc;
@@ -2559,7 +2561,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned 
int idx)
 goto out;
 }
 
-p2m->default_access = hostp2m->default_access;
+p2m->default_access = hvmmem_default_access;
 p2m->domain = hostp2m->domain;
 p2m->global_logdirty = hostp2m->global_logdirty;
 p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
@@ -2576,6 +2578,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned 
int idx)
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
 int rc = -EINVAL;
+struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
 if ( idx >= MAX_ALTP2M )
 return rc;
@@ -2583,16 +2586,22 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned 
int idx)
 altp2m_list_lock(d);
 
 if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
-rc = p2m_activate_altp2m(d, idx);
+rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
 
 altp2m_list_unlock(d);
 return rc;
 }
 
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+ xenmem_access_t hvmmem_default_access)
 {
 int rc = -EINVAL;
 unsigned int i;
+p2m_access_t a;
+struct p2m_domain *p2m;
+
+if ( hvmmem_default_access > XENMEM_access_default )
+return rc;
 
 altp2m_list_lock(d);
 
@@ -2601,7 +2610,15 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
 if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
 continue;
 
-rc = p2m_activate_altp2m(d, i);
+p2m = d->arch.altp2m_p2m[i];
+
+if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, ) )
+{
+altp2m_list_unlock(d);
+return -EINVAL;
+}
+
+rc = p2m_activate_altp2m(d, i, a);
 
 if ( !rc )
 *idx = i;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 94285db1b4..ac2d2787f4 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -884,7 +884,8 @@ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, 
unsigned long gfn_l,
 int