diff mbox series

[V2,2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view

Message ID 20191106153442.12776-2-aisaila@bitdefender.com (mailing list archive)
State Superseded
Headers show
Series [V2,1/2] x86/altp2m: Add hypercall to set a range of sve bits | expand

Commit Message

Alexandru Stefan ISAILA Nov. 6, 2019, 3:35 p.m. UTC 
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 <aisaila@bitdefender.com>
Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/hvm/hvm.c            |  3 ++-
 xen/arch/x86/mm/p2m-ept.c         |  5 +++--
 xen/arch/x86/mm/p2m.c             | 31 ++++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 ++-
 xen/include/asm-x86/p2m.h         |  3 ++-
 xen/include/public/hvm/hvm_op.h   |  2 --
 6 files changed, 35 insertions(+), 12 deletions(-)

Comments

Jan Beulich Nov. 12, 2019, 12:02 p.m. UTC | #1 
On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1345,13 +1345,14 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
> +                         p2m_access_t default_access)
>  {
>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>      struct ept_data *ept;
>  
> -    p2m->default_access = hostp2m->default_access;
> +    p2m->default_access = default_access;
>      p2m->domain = hostp2m->domain;
>  
>      p2m->global_logdirty = hostp2m->global_logdirty;

All of this is not EPT-specific. Before adding more infrastructure to
cover for this (here: another function parameter), how about moving
these parts into vendor-independent code?

> @@ -2572,17 +2574,36 @@ 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,
> +                         uint16_t hvmmem_default_access)
>  {
>      int rc = -EINVAL;
>      unsigned int i;
>  
> +    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),
> +        ACCESS(rx2rw),
> +        ACCESS(n2rwx),
> +#undef ACCESS
> +    };
> +
> +    if ( hvmmem_default_access > XENMEM_access_default )
> +        return rc;
> +
>      altp2m_list_lock(d);
>  
>      for ( i = 0; i < MAX_ALTP2M; i++ )
> @@ -2590,7 +2611,7 @@ 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);
> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);

Aren't you open-coding xenmem_access_to_p2m_access() here? In
no event should there be two instances of the same static array.

Jan
Alexandru Stefan ISAILA Nov. 18, 2019, 8:38 a.m. UTC | #2 
On 12.11.2019 14:02, Jan Beulich wrote:
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1345,13 +1345,14 @@ void setup_ept_dump(void)
>>       register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>>   }
>>   
>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
>> +                         p2m_access_t default_access)
>>   {
>>       struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>       struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>       struct ept_data *ept;
>>   
>> -    p2m->default_access = hostp2m->default_access;
>> +    p2m->default_access = default_access;
>>       p2m->domain = hostp2m->domain;
>>   
>>       p2m->global_logdirty = hostp2m->global_logdirty;
> 
> All of this is not EPT-specific. Before adding more infrastructure to
> cover for this (here: another function parameter), how about moving
> these parts into vendor-independent code?

Ok, I will move the non ept code in p2m_activate_altp2m().

> 
>> @@ -2572,17 +2574,36 @@ 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,
>> +                         uint16_t hvmmem_default_access)
>>   {
>>       int rc = -EINVAL;
>>       unsigned int i;
>>   
>> +    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),
>> +        ACCESS(rx2rw),
>> +        ACCESS(n2rwx),
>> +#undef ACCESS
>> +    };
>> +
>> +    if ( hvmmem_default_access > XENMEM_access_default )
>> +        return rc;
>> +
>>       altp2m_list_lock(d);
>>   
>>       for ( i = 0; i < MAX_ALTP2M; i++ )
>> @@ -2590,7 +2611,7 @@ 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);
>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
> 
> Aren't you open-coding xenmem_access_to_p2m_access() here? In
> no event should there be two instances of the same static array.

I did this because xenmem_access_to_p2m_access() is defined static in 
x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then 
I can go with that and drop this part of the code.

Alex
Jan Beulich Nov. 18, 2019, 9:53 a.m. UTC | #3 
On 18.11.2019 09:38, Alexandru Stefan ISAILA wrote:
> On 12.11.2019 14:02, Jan Beulich wrote:
>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>> @@ -2572,17 +2574,36 @@ 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,
>>> +                         uint16_t hvmmem_default_access)
>>>   {
>>>       int rc = -EINVAL;
>>>       unsigned int i;
>>>   
>>> +    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),
>>> +        ACCESS(rx2rw),
>>> +        ACCESS(n2rwx),
>>> +#undef ACCESS
>>> +    };
>>> +
>>> +    if ( hvmmem_default_access > XENMEM_access_default )
>>> +        return rc;
>>> +
>>>       altp2m_list_lock(d);
>>>   
>>>       for ( i = 0; i < MAX_ALTP2M; i++ )
>>> @@ -2590,7 +2611,7 @@ 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);
>>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
>>
>> Aren't you open-coding xenmem_access_to_p2m_access() here? In
>> no event should there be two instances of the same static array.
> 
> I did this because xenmem_access_to_p2m_access() is defined static in 
> x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then 
> I can go with that and drop this part of the code.

I see no reason why this wouldn't be a reasonable step, allowing to
avoid code duplication. Looks like the function is even suitably
named already for making non-static.

Jan
Tamas K Lengyel Nov. 19, 2019, 7:31 p.m. UTC | #4 
On Mon, Nov 18, 2019 at 2:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.11.2019 09:38, Alexandru Stefan ISAILA wrote:
> > On 12.11.2019 14:02, Jan Beulich wrote:
> >> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> >>> @@ -2572,17 +2574,36 @@ 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,
> >>> +                         uint16_t hvmmem_default_access)
> >>>   {
> >>>       int rc = -EINVAL;
> >>>       unsigned int i;
> >>>
> >>> +    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),
> >>> +        ACCESS(rx2rw),
> >>> +        ACCESS(n2rwx),
> >>> +#undef ACCESS
> >>> +    };
> >>> +
> >>> +    if ( hvmmem_default_access > XENMEM_access_default )
> >>> +        return rc;
> >>> +
> >>>       altp2m_list_lock(d);
> >>>
> >>>       for ( i = 0; i < MAX_ALTP2M; i++ )
> >>> @@ -2590,7 +2611,7 @@ 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);
> >>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
> >>
> >> Aren't you open-coding xenmem_access_to_p2m_access() here? In
> >> no event should there be two instances of the same static array.
> >
> > I did this because xenmem_access_to_p2m_access() is defined static in
> > x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then
> > I can go with that and drop this part of the code.
>
> I see no reason why this wouldn't be a reasonable step, allowing to
> avoid code duplication. Looks like the function is even suitably
> named already for making non-static.

Sounds fine to me too.

Thanks,
Tamas
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 66ed8b8e3e..84f836a5c9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4669,7 +4669,8 @@  static int do_altp2m_op(
     }
 
     case HVMOP_altp2m_create_p2m:
-        if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view)) )
+        if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view,
+                                         a.u.view.hvmmem_default_access)) )
             rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         break;
 
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 220990f017..e62e749ec5 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1345,13 +1345,14 @@  void setup_ept_dump(void)
     register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
 }
 
-void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
+void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
+                         p2m_access_t default_access)
 {
     struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
-    p2m->default_access = hostp2m->default_access;
+    p2m->default_access = default_access;
     p2m->domain = hostp2m->domain;
 
     p2m->global_logdirty = hostp2m->global_logdirty;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9e1335065d..e45a2c3c44 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2528,7 +2528,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;
@@ -2554,7 +2555,7 @@  static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
         goto out;
     }
 
-    p2m_init_altp2m_ept(d, idx);
+    p2m_init_altp2m_ept(d, idx, hvmmem_default_access);
 
  out:
     p2m_unlock(p2m);
@@ -2565,6 +2566,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;
@@ -2572,17 +2574,36 @@  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,
+                         uint16_t hvmmem_default_access)
 {
     int rc = -EINVAL;
     unsigned int i;
 
+    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),
+        ACCESS(rx2rw),
+        ACCESS(n2rwx),
+#undef ACCESS
+    };
+
+    if ( hvmmem_default_access > XENMEM_access_default )
+        return rc;
+
     altp2m_list_lock(d);
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
@@ -2590,7 +2611,7 @@  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);
+        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
 
         if ( !rc )
             *idx = i;
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index ebaa74449b..a9601e8f7e 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -598,7 +598,8 @@  void ept_p2m_uninit(struct p2m_domain *p2m);
 void ept_walk_table(struct domain *d, unsigned long gfn);
 bool_t ept_handle_misconfig(uint64_t gpa);
 void setup_ept_dump(void);
-void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
+void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
+                         p2m_access_t default_access);
 /* Locate an alternate p2m by its EPTP */
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 94285db1b4..321d5e70a8 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 p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);
 
 /* Find an available alternate p2m and make it valid */
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx);
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+                         uint16_t hvmmem_default_access);
 
 /* Make a specific alternate p2m invalid */
 int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx);
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 9834ce0aea..fbe4b53d8d 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -242,8 +242,6 @@  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_vcpu_disable_notify_t);
 struct xen_hvm_altp2m_view {
     /* IN/OUT variable */
     uint16_t view;
-    /* Create view only: default access type
-     * NOTE: currently ignored */
     uint16_t hvmmem_default_access; /* xenmem_access_t */
 };
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;