[Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

By default the sve bits are not set.
This patch adds the option of setting the sve bits upon creating a new
altp2m view.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 tools/libxc/include/xenctrl.h     |  3 +++
 tools/libxc/xc_altp2m.c           | 28 ++++++++++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c            |  3 ++-
 xen/arch/x86/mm/p2m-ept.c         | 19 ++++++++++++++++++-
 xen/arch/x86/mm/p2m.c             | 10 +++++-----
 xen/include/asm-x86/hvm/vmx/vmx.h |  2 +-
 xen/include/asm-x86/p2m.h         |  2 +-
 xen/include/public/hvm/hvm_op.h   |  1 +
 8 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 0ff6ed9e70..86702e5df8 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1925,6 +1925,9 @@ int xc_altp2m_set_vcpu_disable_notify(xc_interface *handle, uint32_t domid,
                                       uint32_t vcpuid);
 int xc_altp2m_create_view(xc_interface *handle, uint32_t domid,
                           xenmem_access_t default_access, uint16_t *view_id);
+int xc_altp2m_create_view_set_sve(xc_interface *handle, uint32_t domid,
+                                  xenmem_access_t default_access,
+                                  uint16_t *view_id, bool set_sve);
 int xc_altp2m_destroy_view(xc_interface *handle, uint32_t domid,
                            uint16_t view_id);
 /* Switch all vCPUs of the domain to the specified altp2m view */
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index a86520c232..aeb9f36ea0 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -139,6 +139,34 @@ int xc_altp2m_create_view(xc_interface *handle, uint32_t domid,
     return rc;
 }
 
+int xc_altp2m_create_view_set_sve(xc_interface *handle, uint32_t domid,
+                                  xenmem_access_t default_access,
+                                  uint16_t *view_id, bool set_sve)
+{
+    int rc;
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_create_p2m;
+    arg->domain = domid;
+    arg->u.view.view = -1;
+    arg->u.view.hvmmem_default_access = default_access;
+    arg->u.view.set_sve = set_sve;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+                  HYPERCALL_BUFFER_AS_ARG(arg));
+
+    if ( !rc )
+        *view_id = arg->u.view.view;
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
+
 int xc_altp2m_destroy_view(xc_interface *handle, uint32_t domid,
                            uint16_t view_id)
 {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 029eea3b85..95d382b114 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4627,7 +4627,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.set_sve)) )
             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 6b8468c793..255ed97734 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1339,7 +1339,7 @@ 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, bool set_sve)
 {
     struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
@@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     ept = &p2m->ept;
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
     d->arch.altp2m_eptp[i] = ept->eptp;
+
+    if ( set_sve )
+    {
+        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
+
+        for( ; gfn < max_gpfn; ++gfn )
+        {
+            mfn_t mfn;
+            p2m_access_t a;
+            p2m_type_t t;
+
+            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
+                                       AP2MGET_query);
+            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
+
+        }
+    }
 }
 
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 8a5229ee21..4ec61740f6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2528,7 +2528,7 @@ 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, bool set_sve)
 {
     struct p2m_domain *hostp2m, *p2m;
     int rc;
@@ -2554,7 +2554,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, set_sve);
 
  out:
     p2m_unlock(p2m);
@@ -2572,13 +2572,13 @@ 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, false);
 
     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, bool set_sve)
 {
     int rc = -EINVAL;
     unsigned int i;
@@ -2590,7 +2590,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, set_sve);
 
         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..7707f1768f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -598,7 +598,7 @@ 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, bool set_sve);
 /* 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..c85c91819e 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -884,7 +884,7 @@ 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, bool set_sve);
 
 /* 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 c6cd12f596..924d947e78 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
     /* Create view only: default access type
      * NOTE: currently ignored */
     uint16_t hvmmem_default_access; /* xenmem_access_t */
+    uint8_t set_sve; /* bool value */
 };
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Jan Beulich]

On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>      ept = &p2m->ept;
>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>      d->arch.altp2m_eptp[i] = ept->eptp;
> +
> +    if ( set_sve )
> +    {
> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
> +
> +        for( ; gfn < max_gpfn; ++gfn )
> +        {
> +            mfn_t mfn;
> +            p2m_access_t a;
> +            p2m_type_t t;
> +
> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
> +                                       AP2MGET_query);
> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
> +
> +        }
> +    }
>  }

How long is this loop going to take for a huge guest? IOW how
come there's no preemption in here, or some other mechanism
to bound execution time?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>      /* Create view only: default access type
>       * NOTE: currently ignored */
>      uint16_t hvmmem_default_access; /* xenmem_access_t */
> +    uint8_t set_sve; /* bool value */
>  };

This interface is, given the right configuration, available to
guests. Hence you can't simply add a field here. Just consider
what happens for an existing caller when there is random data
in the field you now assign a meaning.

Furthermore, according to common practice elsewhere, the new
trailing padding field should be made explicit, and checked to
hold zero on input.

Jan

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Tamas K Lengyel]

On Tue, Sep 3, 2019 at 9:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
> > @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> >      ept = &p2m->ept;
> >      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
> >      d->arch.altp2m_eptp[i] = ept->eptp;
> > +
> > +    if ( set_sve )
> > +    {
> > +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
> > +
> > +        for( ; gfn < max_gpfn; ++gfn )
> > +        {
> > +            mfn_t mfn;
> > +            p2m_access_t a;
> > +            p2m_type_t t;
> > +
> > +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
> > +                                       AP2MGET_query);
> > +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
> > +
> > +        }
> > +    }
> >  }
>
> How long is this loop going to take for a huge guest? IOW how
> come there's no preemption in here, or some other mechanism
> to bound execution time?

Also, looks to me you should check whether the mfn is valid before
calling p2m->set_entry.

>
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
> >      /* Create view only: default access type
> >       * NOTE: currently ignored */
> >      uint16_t hvmmem_default_access; /* xenmem_access_t */
> > +    uint8_t set_sve; /* bool value */
> >  };
>
> This interface is, given the right configuration, available to
> guests. Hence you can't simply add a field here. Just consider
> what happens for an existing caller when there is random data
> in the field you now assign a meaning.

Perhaps instead of extending the HVMOP it would make more sense to
just add a xl config option that defines the "default" sve bit for
altp2m views in the domain?

Tamas

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

On 03.09.2019 20:24, Tamas K Lengyel wrote:
> On Tue, Sep 3, 2019 at 9:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>       ept = &p2m->ept;
>>>       ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>       d->arch.altp2m_eptp[i] = ept->eptp;
>>> +
>>> +    if ( set_sve )
>>> +    {
>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>> +
>>> +        for( ; gfn < max_gpfn; ++gfn )
>>> +        {
>>> +            mfn_t mfn;
>>> +            p2m_access_t a;
>>> +            p2m_type_t t;
>>> +
>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>> +                                       AP2MGET_query);
>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>> +
>>> +        }
>>> +    }
>>>   }
>>
>> How long is this loop going to take for a huge guest? IOW how
>> come there's no preemption in here, or some other mechanism
>> to bound execution time?
> 
> Also, looks to me you should check whether the mfn is valid before
> calling p2m->set_entry.

I agree, I will check the mfn in the next version.

> 
>>
>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>>>       /* Create view only: default access type
>>>        * NOTE: currently ignored */
>>>       uint16_t hvmmem_default_access; /* xenmem_access_t */
>>> +    uint8_t set_sve; /* bool value */
>>>   };
>>
>> This interface is, given the right configuration, available to
>> guests. Hence you can't simply add a field here. Just consider
>> what happens for an existing caller when there is random data
>> in the field you now assign a meaning.
> 
> Perhaps instead of extending the HVMOP it would make more sense to
> just add a xl config option that defines the "default" sve bit for
> altp2m views in the domain?
> 

Adding a xl config option will not work for systems that do use xl. 
There is a need that this will work in all cases.

Thanks and sorry for replying so late,

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Roger Pau Monné]

On Wed, Oct 23, 2019 at 09:11:54AM +0000, Alexandru Stefan ISAILA wrote:
> 
> 
> On 03.09.2019 20:24, Tamas K Lengyel wrote:
> > On Tue, Sep 3, 2019 at 9:53 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
> >>> --- a/xen/include/public/hvm/hvm_op.h
> >>> +++ b/xen/include/public/hvm/hvm_op.h
> >>> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
> >>>       /* Create view only: default access type
> >>>        * NOTE: currently ignored */
> >>>       uint16_t hvmmem_default_access; /* xenmem_access_t */
> >>> +    uint8_t set_sve; /* bool value */
> >>>   };
> >>
> >> This interface is, given the right configuration, available to
> >> guests. Hence you can't simply add a field here. Just consider
> >> what happens for an existing caller when there is random data
> >> in the field you now assign a meaning.
> > 
> > Perhaps instead of extending the HVMOP it would make more sense to
> > just add a xl config option that defines the "default" sve bit for
> > altp2m views in the domain?
> > 
> 
> Adding a xl config option will not work for systems that do use xl. 
> There is a need that this will work in all cases.

I assume that such option would be implemented using a DOMCTL, which
can also be used by other toolstacks. I however have no idea whether
this is a suitable interface or not for this feature.

Roger.

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

On 23.10.2019 14:58, Roger Pau Monné wrote:
> On Wed, Oct 23, 2019 at 09:11:54AM +0000, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 03.09.2019 20:24, Tamas K Lengyel wrote:
>>> On Tue, Sep 3, 2019 at 9:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>>>>>        /* Create view only: default access type
>>>>>         * NOTE: currently ignored */
>>>>>        uint16_t hvmmem_default_access; /* xenmem_access_t */
>>>>> +    uint8_t set_sve; /* bool value */
>>>>>    };
>>>>
>>>> This interface is, given the right configuration, available to
>>>> guests. Hence you can't simply add a field here. Just consider
>>>> what happens for an existing caller when there is random data
>>>> in the field you now assign a meaning.
>>>
>>> Perhaps instead of extending the HVMOP it would make more sense to
>>> just add a xl config option that defines the "default" sve bit for
>>> altp2m views in the domain?
>>>
>>
>> Adding a xl config option will not work for systems that do use xl.
>> There is a need that this will work in all cases.
> 
> I assume that such option would be implemented using a DOMCTL, which
> can also be used by other toolstacks. I however have no idea whether
> this is a suitable interface or not for this feature.
> 

I think that having a HVMOP_altp2m_get_suppress_ve_multi and letting the 
caller provide the start gfn and the nr of pages to have the sve bits 
set will provide a good solution for init an dfor further development.

I will go on this way for version 2 if everyone is ok with this.

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

On 25.10.2019 17:36, Alexandru Stefan ISAILA wrote:
> 
> 
> On 23.10.2019 14:58, Roger Pau Monné wrote:
>> On Wed, Oct 23, 2019 at 09:11:54AM +0000, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 03.09.2019 20:24, Tamas K Lengyel wrote:
>>>> On Tue, Sep 3, 2019 at 9:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>>> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>>>>>>         /* Create view only: default access type
>>>>>>          * NOTE: currently ignored */
>>>>>>         uint16_t hvmmem_default_access; /* xenmem_access_t */
>>>>>> +    uint8_t set_sve; /* bool value */
>>>>>>     };
>>>>>
>>>>> This interface is, given the right configuration, available to
>>>>> guests. Hence you can't simply add a field here. Just consider
>>>>> what happens for an existing caller when there is random data
>>>>> in the field you now assign a meaning.
>>>>
>>>> Perhaps instead of extending the HVMOP it would make more sense to
>>>> just add a xl config option that defines the "default" sve bit for
>>>> altp2m views in the domain?
>>>>
>>>
>>> Adding a xl config option will not work for systems that do use xl.
>>> There is a need that this will work in all cases.
>>
>> I assume that such option would be implemented using a DOMCTL, which
>> can also be used by other toolstacks. I however have no idea whether
>> this is a suitable interface or not for this feature.
>>
> 
> I think that having a HVMOP_altp2m_get_suppress_ve_multi and letting the
HVMOP_altp2m_set_suppress_ve_multi (sorry for the typo)
> caller provide the start gfn and the nr of pages to have the sve bits
> set will provide a good solution for init an dfor further development.
> 
> I will go on this way for version 2 if everyone is ok with this.
> 
> Thanks,
> Alex
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> ________________________
> This email was scanned by Bitdefender
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

On 03.09.2019 18:52, Jan Beulich wrote:
> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>       ept = &p2m->ept;
>>       ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>       d->arch.altp2m_eptp[i] = ept->eptp;
>> +
>> +    if ( set_sve )
>> +    {
>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>> +
>> +        for( ; gfn < max_gpfn; ++gfn )
>> +        {
>> +            mfn_t mfn;
>> +            p2m_access_t a;
>> +            p2m_type_t t;
>> +
>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>> +                                       AP2MGET_query);
>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>> +
>> +        }
>> +    }
>>   }
> 
> How long is this loop going to take for a huge guest? IOW how
> come there's no preemption in here, or some other mechanism
> to bound execution time?

Because this is done for the initialization of a new view and the p2m is 
locked.

Alex

> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>>       /* Create view only: default access type
>>        * NOTE: currently ignored */
>>       uint16_t hvmmem_default_access; /* xenmem_access_t */
>> +    uint8_t set_sve; /* bool value */
>>   };
> 
> This interface is, given the right configuration, available to
> guests. Hence you can't simply add a field here. Just consider
> what happens for an existing caller when there is random data
> in the field you now assign a meaning.
> 
> Furthermore, according to common practice elsewhere, the new
> trailing padding field should be made explicit, and checked to
> hold zero on input.
> 
> Jan

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Jan Beulich]

On 04.09.2019 13:51, Alexandru Stefan ISAILA wrote:
> 
> 
> On 03.09.2019 18:52, Jan Beulich wrote:
>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>       ept = &p2m->ept;
>>>       ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>       d->arch.altp2m_eptp[i] = ept->eptp;
>>> +
>>> +    if ( set_sve )
>>> +    {
>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>> +
>>> +        for( ; gfn < max_gpfn; ++gfn )
>>> +        {
>>> +            mfn_t mfn;
>>> +            p2m_access_t a;
>>> +            p2m_type_t t;
>>> +
>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>> +                                       AP2MGET_query);
>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>> +
>>> +        }
>>> +    }
>>>   }
>>
>> How long is this loop going to take for a huge guest? IOW how
>> come there's no preemption in here, or some other mechanism
>> to bound execution time?
> 
> Because this is done for the initialization of a new view and the p2m is 
> locked.

Well, this makes handling this the way you want it close to
impossible, but is not an argument against the need for preemption
here. Just like it had turned out to be unreasonable to
preemptively handle other P2M adjustments (which is why
p2m-ept.c:resolve_misconfig() and p2m-pt.c:do_recalc() got
introduced), I'm afraid you'll have to use some other technique
here (possibly building on top of the mentioned functions).

Jan

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

On 04.09.2019 15:14, Jan Beulich wrote:
> On 04.09.2019 13:51, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 03.09.2019 18:52, Jan Beulich wrote:
>>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>        ept = &p2m->ept;
>>>>        ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>>        d->arch.altp2m_eptp[i] = ept->eptp;
>>>> +
>>>> +    if ( set_sve )
>>>> +    {
>>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>>> +
>>>> +        for( ; gfn < max_gpfn; ++gfn )
>>>> +        {
>>>> +            mfn_t mfn;
>>>> +            p2m_access_t a;
>>>> +            p2m_type_t t;
>>>> +
>>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>>> +                                       AP2MGET_query);
>>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>>> +
>>>> +        }
>>>> +    }
>>>>    }
>>>
>>> How long is this loop going to take for a huge guest? IOW how
>>> come there's no preemption in here, or some other mechanism
>>> to bound execution time?
>>
>> Because this is done for the initialization of a new view and the p2m is
>> locked.
> 
> Well, this makes handling this the way you want it close to
> impossible, but is not an argument against the need for preemption
> here. Just like it had turned out to be unreasonable to
> preemptively handle other P2M adjustments (which is why
> p2m-ept.c:resolve_misconfig() and p2m-pt.c:do_recalc() got
> introduced), I'm afraid you'll have to use some other technique
> here (possibly building on top of the mentioned functions).
> 

I think that the mechanism from p2m_set_mem_access_multi() can suit this 
case, start the loop, set ,if(hypercall_preempt_check()) rc = 
next_unset_gfn;

And for this to work it should have a new "start_gfn" parameter so the 
caller can issue multiple hypercalls until gfn == max_gfn.

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Jan Beulich]

On 04.09.2019 15:04, Alexandru Stefan ISAILA wrote:
> 
> 
> On 04.09.2019 15:14, Jan Beulich wrote:
>> On 04.09.2019 13:51, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 03.09.2019 18:52, Jan Beulich wrote:
>>>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>>        ept = &p2m->ept;
>>>>>        ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>>>        d->arch.altp2m_eptp[i] = ept->eptp;
>>>>> +
>>>>> +    if ( set_sve )
>>>>> +    {
>>>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>>>> +
>>>>> +        for( ; gfn < max_gpfn; ++gfn )
>>>>> +        {
>>>>> +            mfn_t mfn;
>>>>> +            p2m_access_t a;
>>>>> +            p2m_type_t t;
>>>>> +
>>>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>>>> +                                       AP2MGET_query);
>>>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>>>> +
>>>>> +        }
>>>>> +    }
>>>>>    }
>>>>
>>>> How long is this loop going to take for a huge guest? IOW how
>>>> come there's no preemption in here, or some other mechanism
>>>> to bound execution time?
>>>
>>> Because this is done for the initialization of a new view and the p2m is
>>> locked.
>>
>> Well, this makes handling this the way you want it close to
>> impossible, but is not an argument against the need for preemption
>> here. Just like it had turned out to be unreasonable to
>> preemptively handle other P2M adjustments (which is why
>> p2m-ept.c:resolve_misconfig() and p2m-pt.c:do_recalc() got
>> introduced), I'm afraid you'll have to use some other technique
>> here (possibly building on top of the mentioned functions).
>>
> 
> I think that the mechanism from p2m_set_mem_access_multi() can suit this 
> case, start the loop, set ,if(hypercall_preempt_check()) rc = 
> next_unset_gfn;
> 
> And for this to work it should have a new "start_gfn" parameter so the 
> caller can issue multiple hypercalls until gfn == max_gfn.

Hmm, possible. I took your previous reply to mean that it is
important for the p2m to not get unlocked in the middle of this
process. If this was a wrong understanding of mine, then yes,
"conventional" preemption like you outline it ought to work.

Jan

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA]

On 04.09.2019 16:17, Jan Beulich wrote:
> On 04.09.2019 15:04, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 04.09.2019 15:14, Jan Beulich wrote:
>>> On 04.09.2019 13:51, Alexandru Stefan ISAILA wrote:
>>>>
>>>>
>>>> On 03.09.2019 18:52, Jan Beulich wrote:
>>>>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>>>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>>>         ept = &p2m->ept;
>>>>>>         ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>>>>         d->arch.altp2m_eptp[i] = ept->eptp;
>>>>>> +
>>>>>> +    if ( set_sve )
>>>>>> +    {
>>>>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>>>>> +
>>>>>> +        for( ; gfn < max_gpfn; ++gfn )
>>>>>> +        {
>>>>>> +            mfn_t mfn;
>>>>>> +            p2m_access_t a;
>>>>>> +            p2m_type_t t;
>>>>>> +
>>>>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>>>>> +                                       AP2MGET_query);
>>>>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>>>>> +
>>>>>> +        }
>>>>>> +    }
>>>>>>     }
>>>>>
>>>>> How long is this loop going to take for a huge guest? IOW how
>>>>> come there's no preemption in here, or some other mechanism
>>>>> to bound execution time?
>>>>
>>>> Because this is done for the initialization of a new view and the p2m is
>>>> locked.
>>>
>>> Well, this makes handling this the way you want it close to
>>> impossible, but is not an argument against the need for preemption
>>> here. Just like it had turned out to be unreasonable to
>>> preemptively handle other P2M adjustments (which is why
>>> p2m-ept.c:resolve_misconfig() and p2m-pt.c:do_recalc() got
>>> introduced), I'm afraid you'll have to use some other technique
>>> here (possibly building on top of the mentioned functions).
>>>
>>
>> I think that the mechanism from p2m_set_mem_access_multi() can suit this
>> case, start the loop, set ,if(hypercall_preempt_check()) rc =
>> next_unset_gfn;
>>
>> And for this to work it should have a new "start_gfn" parameter so the
>> caller can issue multiple hypercalls until gfn == max_gfn.
> 
> Hmm, possible. I took your previous reply to mean that it is
> important for the p2m to not get unlocked in the middle of this
> process. If this was a wrong understanding of mine, then yes,
> "conventional" preemption like you outline it ought to work.
> 

There are two possible ways to solve this:
1. the conventional preemption way and with this I do not return ok on 
the init function until the final gfn has the bit set (code will have to 
change to accommodate this new option/preemption).

2. as you described and work with p2m->recalc() (I will have to check 
this out to have a correct idea of what is to be done).

What do you think is the preferred way to continue with this?

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Jan Beulich]

On 04.09.2019 16:14, Alexandru Stefan ISAILA wrote:
> 
> 
> On 04.09.2019 16:17, Jan Beulich wrote:
>> On 04.09.2019 15:04, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 04.09.2019 15:14, Jan Beulich wrote:
>>>> On 04.09.2019 13:51, Alexandru Stefan ISAILA wrote:
>>>>>
>>>>>
>>>>> On 03.09.2019 18:52, Jan Beulich wrote:
>>>>>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>>>>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>>>>         ept = &p2m->ept;
>>>>>>>         ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>>>>>         d->arch.altp2m_eptp[i] = ept->eptp;
>>>>>>> +
>>>>>>> +    if ( set_sve )
>>>>>>> +    {
>>>>>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>>>>>> +
>>>>>>> +        for( ; gfn < max_gpfn; ++gfn )
>>>>>>> +        {
>>>>>>> +            mfn_t mfn;
>>>>>>> +            p2m_access_t a;
>>>>>>> +            p2m_type_t t;
>>>>>>> +
>>>>>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>>>>>> +                                       AP2MGET_query);
>>>>>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>>>>>> +
>>>>>>> +        }
>>>>>>> +    }
>>>>>>>     }
>>>>>>
>>>>>> How long is this loop going to take for a huge guest? IOW how
>>>>>> come there's no preemption in here, or some other mechanism
>>>>>> to bound execution time?
>>>>>
>>>>> Because this is done for the initialization of a new view and the p2m is
>>>>> locked.
>>>>
>>>> Well, this makes handling this the way you want it close to
>>>> impossible, but is not an argument against the need for preemption
>>>> here. Just like it had turned out to be unreasonable to
>>>> preemptively handle other P2M adjustments (which is why
>>>> p2m-ept.c:resolve_misconfig() and p2m-pt.c:do_recalc() got
>>>> introduced), I'm afraid you'll have to use some other technique
>>>> here (possibly building on top of the mentioned functions).
>>>>
>>>
>>> I think that the mechanism from p2m_set_mem_access_multi() can suit this
>>> case, start the loop, set ,if(hypercall_preempt_check()) rc =
>>> next_unset_gfn;
>>>
>>> And for this to work it should have a new "start_gfn" parameter so the
>>> caller can issue multiple hypercalls until gfn == max_gfn.
>>
>> Hmm, possible. I took your previous reply to mean that it is
>> important for the p2m to not get unlocked in the middle of this
>> process. If this was a wrong understanding of mine, then yes,
>> "conventional" preemption like you outline it ought to work.
>>
> 
> There are two possible ways to solve this:
> 1. the conventional preemption way and with this I do not return ok on 
> the init function until the final gfn has the bit set (code will have to 
> change to accommodate this new option/preemption).
> 
> 2. as you described and work with p2m->recalc() (I will have to check 
> this out to have a correct idea of what is to be done).
> 
> What do you think is the preferred way to continue with this?

Unless I'm missing information on the requirements I think either
would be fine. If the change was much simpler using the recalc
code, perhaps that'd be the way to go. In all other cases I'd
suggest to try to leave the already non-trivial recalc code alone.

Jan

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

Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Julien Grall]

Hi,

On 02/09/2019 09:11, Alexandru Stefan ISAILA wrote:
> By default the sve bits are not set.

While altp2m is currently only supported for x86 (though I am aware of 
some port for Arm), this patch extends an interface that is meant to be 
arch-agnostic. So what does "sve" stands for?

> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index c6cd12f596..924d947e78 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>       /* Create view only: default access type
>        * NOTE: currently ignored */
>       uint16_t hvmmem_default_access; /* xenmem_access_t */
> +    uint8_t set_sve; /* bool value */

Do we expect more "default" to be override? It might be worth 
considering to introduce flags rather than waste 8-bits per new default 
that can be encoded in 1-bit.

Cheers

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