| Message ID | 20160801171028.11615-2-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, On 01/08/16 18:10, Sergej Proskurin wrote: > This commit moves the altp2m-related code from x86 to ARM. Functions > that are no yet supported notify the caller or print a BUG message > stating their absence. > > Also, the struct arch_domain is extended with the altp2m_active > attribute, representing the current altp2m activity configuration of the > domain. > > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. > Removed not used altp2m helper stubs in altp2m.h. > --- > xen/arch/arm/hvm.c | 79 ++++++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/altp2m.h | 4 +-- > xen/include/asm-arm/domain.h | 3 ++ > 3 files changed, 84 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index d999bde..eb524ae 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -32,6 +32,81 @@ > > #include <asm/hypercall.h> > > +#include <asm/altp2m.h> > + > +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) > +{ > + struct xen_hvm_altp2m_op a; > + struct domain *d = NULL; > + int rc = 0; > + > + if ( copy_from_guest(&a, arg, 1) ) > + return -EFAULT; > + > + if ( a.pad1 || a.pad2 || > + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || > + (a.cmd < HVMOP_altp2m_get_domain_state) || > + (a.cmd > HVMOP_altp2m_change_gfn) ) > + return -EINVAL; > + > + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? > + rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); > + > + if ( d == NULL ) > + return -ESRCH; > + > + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && > + (a.cmd != HVMOP_altp2m_set_domain_state) && > + !d->arch.altp2m_active ) Why not using altp2m_active(d) here? Also this check looks quite racy. What does prevent another CPU to disable altp2m at the same time? How the code would behave? Regards,
Hi Julien, On 08/03/2016 06:54 PM, Julien Grall wrote: > Hello Sergej, > > On 01/08/16 18:10, Sergej Proskurin wrote: >> This commit moves the altp2m-related code from x86 to ARM. Functions >> that are no yet supported notify the caller or print a BUG message >> stating their absence. >> >> Also, the struct arch_domain is extended with the altp2m_active >> attribute, representing the current altp2m activity configuration of the >> domain. >> >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. >> Removed not used altp2m helper stubs in altp2m.h. >> --- >> xen/arch/arm/hvm.c | 79 >> ++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/altp2m.h | 4 +-- >> xen/include/asm-arm/domain.h | 3 ++ >> 3 files changed, 84 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index d999bde..eb524ae 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -32,6 +32,81 @@ >> >> #include <asm/hypercall.h> >> >> +#include <asm/altp2m.h> >> + >> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >> +{ >> + struct xen_hvm_altp2m_op a; >> + struct domain *d = NULL; >> + int rc = 0; >> + >> + if ( copy_from_guest(&a, arg, 1) ) >> + return -EFAULT; >> + >> + if ( a.pad1 || a.pad2 || >> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >> + (a.cmd < HVMOP_altp2m_get_domain_state) || >> + (a.cmd > HVMOP_altp2m_change_gfn) ) >> + return -EINVAL; >> + >> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >> + rcu_lock_domain_by_any_id(a.domain) : >> rcu_lock_current_domain(); >> + >> + if ( d == NULL ) >> + return -ESRCH; >> + >> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >> + (a.cmd != HVMOP_altp2m_set_domain_state) && >> + !d->arch.altp2m_active ) > > Why not using altp2m_active(d) here? > I have already changed that within the next patch version. Thank you. > Also this check looks quite racy. What does prevent another CPU to > disable altp2m at the same time? How the code would behave? > Thank you. I will protect this part with the altp2m_lock. Best regards, ~Sergej
On 04/08/16 17:01, Sergej Proskurin wrote: > Hi Julien, > > > On 08/03/2016 06:54 PM, Julien Grall wrote: >> Hello Sergej, >> >> On 01/08/16 18:10, Sergej Proskurin wrote: >>> This commit moves the altp2m-related code from x86 to ARM. Functions >>> that are no yet supported notify the caller or print a BUG message >>> stating their absence. >>> >>> Also, the struct arch_domain is extended with the altp2m_active >>> attribute, representing the current altp2m activity configuration of the >>> domain. >>> >>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>> --- >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> --- >>> v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. >>> Removed not used altp2m helper stubs in altp2m.h. >>> --- >>> xen/arch/arm/hvm.c | 79 >>> ++++++++++++++++++++++++++++++++++++++++++++ >>> xen/include/asm-arm/altp2m.h | 4 +-- >>> xen/include/asm-arm/domain.h | 3 ++ >>> 3 files changed, 84 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>> index d999bde..eb524ae 100644 >>> --- a/xen/arch/arm/hvm.c >>> +++ b/xen/arch/arm/hvm.c >>> @@ -32,6 +32,81 @@ >>> >>> #include <asm/hypercall.h> >>> >>> +#include <asm/altp2m.h> >>> + >>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>> +{ >>> + struct xen_hvm_altp2m_op a; >>> + struct domain *d = NULL; >>> + int rc = 0; >>> + >>> + if ( copy_from_guest(&a, arg, 1) ) >>> + return -EFAULT; >>> + >>> + if ( a.pad1 || a.pad2 || >>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >>> + return -EINVAL; >>> + >>> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >>> + rcu_lock_domain_by_any_id(a.domain) : >>> rcu_lock_current_domain(); >>> + >>> + if ( d == NULL ) >>> + return -ESRCH; >>> + >>> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >>> + (a.cmd != HVMOP_altp2m_set_domain_state) && >>> + !d->arch.altp2m_active ) >> >> Why not using altp2m_active(d) here? >> > > I have already changed that within the next patch version. Thank you. > >> Also this check looks quite racy. What does prevent another CPU to >> disable altp2m at the same time? How the code would behave? >> > > Thank you. I will protect this part with the altp2m_lock. I have noticed that you use the altp2m_lock (it is a spinlock) in multiple places. So you will serialize a lot of code. Is it fine for you? Regards,
On 08/04/2016 06:04 PM, Julien Grall wrote: > > > On 04/08/16 17:01, Sergej Proskurin wrote: >> Hi Julien, >> >> >> On 08/03/2016 06:54 PM, Julien Grall wrote: >>> Hello Sergej, >>> >>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>> This commit moves the altp2m-related code from x86 to ARM. Functions >>>> that are no yet supported notify the caller or print a BUG message >>>> stating their absence. >>>> >>>> Also, the struct arch_domain is extended with the altp2m_active >>>> attribute, representing the current altp2m activity configuration >>>> of the >>>> domain. >>>> >>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>>> --- >>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>> Cc: Julien Grall <julien.grall@arm.com> >>>> --- >>>> v2: Removed altp2m command-line option: Guard through >>>> HVM_PARAM_ALTP2M. >>>> Removed not used altp2m helper stubs in altp2m.h. >>>> --- >>>> xen/arch/arm/hvm.c | 79 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>> xen/include/asm-arm/altp2m.h | 4 +-- >>>> xen/include/asm-arm/domain.h | 3 ++ >>>> 3 files changed, 84 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>>> index d999bde..eb524ae 100644 >>>> --- a/xen/arch/arm/hvm.c >>>> +++ b/xen/arch/arm/hvm.c >>>> @@ -32,6 +32,81 @@ >>>> >>>> #include <asm/hypercall.h> >>>> >>>> +#include <asm/altp2m.h> >>>> + >>>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>>> +{ >>>> + struct xen_hvm_altp2m_op a; >>>> + struct domain *d = NULL; >>>> + int rc = 0; >>>> + >>>> + if ( copy_from_guest(&a, arg, 1) ) >>>> + return -EFAULT; >>>> + >>>> + if ( a.pad1 || a.pad2 || >>>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >>>> + return -EINVAL; >>>> + >>>> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >>>> + rcu_lock_domain_by_any_id(a.domain) : >>>> rcu_lock_current_domain(); >>>> + >>>> + if ( d == NULL ) >>>> + return -ESRCH; >>>> + >>>> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >>>> + (a.cmd != HVMOP_altp2m_set_domain_state) && >>>> + !d->arch.altp2m_active ) >>> >>> Why not using altp2m_active(d) here? >>> >> >> I have already changed that within the next patch version. Thank you. >> >>> Also this check looks quite racy. What does prevent another CPU to >>> disable altp2m at the same time? How the code would behave? >>> >> >> Thank you. I will protect this part with the altp2m_lock. > > I have noticed that you use the altp2m_lock (it is a spinlock) in > multiple places. So you will serialize a lot of code. Is it fine for you? > I would need to move the lock from altp2m_init_by_id to the outside. This would not lock much more code as it already does. Apart from that, since activating/deactivating altp2m on a specific domain should be used very rarely (including the first time when no altp2m structures are initialized), it is fine to me. Unless, you would like me to use a different lock instead? Best regards, ~Sergej
On 04/08/16 17:22, Sergej Proskurin wrote: > > On 08/04/2016 06:04 PM, Julien Grall wrote: >> >> >> On 04/08/16 17:01, Sergej Proskurin wrote: >>> Hi Julien, >>> >>> >>> On 08/03/2016 06:54 PM, Julien Grall wrote: >>>> Hello Sergej, >>>> >>>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>>> This commit moves the altp2m-related code from x86 to ARM. Functions >>>>> that are no yet supported notify the caller or print a BUG message >>>>> stating their absence. >>>>> >>>>> Also, the struct arch_domain is extended with the altp2m_active >>>>> attribute, representing the current altp2m activity configuration >>>>> of the >>>>> domain. >>>>> >>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>>>> --- >>>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>>> Cc: Julien Grall <julien.grall@arm.com> >>>>> --- >>>>> v2: Removed altp2m command-line option: Guard through >>>>> HVM_PARAM_ALTP2M. >>>>> Removed not used altp2m helper stubs in altp2m.h. >>>>> --- >>>>> xen/arch/arm/hvm.c | 79 >>>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>> xen/include/asm-arm/altp2m.h | 4 +-- >>>>> xen/include/asm-arm/domain.h | 3 ++ >>>>> 3 files changed, 84 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>>>> index d999bde..eb524ae 100644 >>>>> --- a/xen/arch/arm/hvm.c >>>>> +++ b/xen/arch/arm/hvm.c >>>>> @@ -32,6 +32,81 @@ >>>>> >>>>> #include <asm/hypercall.h> >>>>> >>>>> +#include <asm/altp2m.h> >>>>> + >>>>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>>>> +{ >>>>> + struct xen_hvm_altp2m_op a; >>>>> + struct domain *d = NULL; >>>>> + int rc = 0; >>>>> + >>>>> + if ( copy_from_guest(&a, arg, 1) ) >>>>> + return -EFAULT; >>>>> + >>>>> + if ( a.pad1 || a.pad2 || >>>>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>>>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>>>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >>>>> + return -EINVAL; >>>>> + >>>>> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >>>>> + rcu_lock_domain_by_any_id(a.domain) : >>>>> rcu_lock_current_domain(); >>>>> + >>>>> + if ( d == NULL ) >>>>> + return -ESRCH; >>>>> + >>>>> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >>>>> + (a.cmd != HVMOP_altp2m_set_domain_state) && >>>>> + !d->arch.altp2m_active ) >>>> >>>> Why not using altp2m_active(d) here? >>>> >>> >>> I have already changed that within the next patch version. Thank you. >>> >>>> Also this check looks quite racy. What does prevent another CPU to >>>> disable altp2m at the same time? How the code would behave? >>>> >>> >>> Thank you. I will protect this part with the altp2m_lock. >> >> I have noticed that you use the altp2m_lock (it is a spinlock) in >> multiple places. So you will serialize a lot of code. Is it fine for you? >> > > I would need to move the lock from altp2m_init_by_id to the outside. > This would not lock much more code as it already does. Apart from that, > since activating/deactivating altp2m on a specific domain should be used > very rarely (including the first time when no altp2m structures are > initialized), it is fine to me. Unless, you would like me to use a > different lock instead? I don't know, it was an open question. There are a couple of places where you may need to add lock atlp2m_lock (such altp2m_copy_lazy), so you would serialize all the access. If you care about performance, then you may want to use a different lock or method of locking (such as read-write-lock). Regards,
Hi Julien, On 08/04/2016 06:51 PM, Julien Grall wrote: > > > On 04/08/16 17:22, Sergej Proskurin wrote: >> >> On 08/04/2016 06:04 PM, Julien Grall wrote: >>> >>> >>> On 04/08/16 17:01, Sergej Proskurin wrote: >>>> Hi Julien, >>>> >>>> >>>> On 08/03/2016 06:54 PM, Julien Grall wrote: >>>>> Hello Sergej, >>>>> >>>>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>>>> This commit moves the altp2m-related code from x86 to ARM. Functions >>>>>> that are no yet supported notify the caller or print a BUG message >>>>>> stating their absence. >>>>>> >>>>>> Also, the struct arch_domain is extended with the altp2m_active >>>>>> attribute, representing the current altp2m activity configuration >>>>>> of the >>>>>> domain. >>>>>> >>>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>>>>> --- >>>>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>>>> Cc: Julien Grall <julien.grall@arm.com> >>>>>> --- >>>>>> v2: Removed altp2m command-line option: Guard through >>>>>> HVM_PARAM_ALTP2M. >>>>>> Removed not used altp2m helper stubs in altp2m.h. >>>>>> --- >>>>>> xen/arch/arm/hvm.c | 79 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>>> xen/include/asm-arm/altp2m.h | 4 +-- >>>>>> xen/include/asm-arm/domain.h | 3 ++ >>>>>> 3 files changed, 84 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>>>>> index d999bde..eb524ae 100644 >>>>>> --- a/xen/arch/arm/hvm.c >>>>>> +++ b/xen/arch/arm/hvm.c >>>>>> @@ -32,6 +32,81 @@ >>>>>> >>>>>> #include <asm/hypercall.h> >>>>>> >>>>>> +#include <asm/altp2m.h> >>>>>> + >>>>>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>> +{ >>>>>> + struct xen_hvm_altp2m_op a; >>>>>> + struct domain *d = NULL; >>>>>> + int rc = 0; >>>>>> + >>>>>> + if ( copy_from_guest(&a, arg, 1) ) >>>>>> + return -EFAULT; >>>>>> + >>>>>> + if ( a.pad1 || a.pad2 || >>>>>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>>>>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>>>>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >>>>>> + rcu_lock_domain_by_any_id(a.domain) : >>>>>> rcu_lock_current_domain(); >>>>>> + >>>>>> + if ( d == NULL ) >>>>>> + return -ESRCH; >>>>>> + >>>>>> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >>>>>> + (a.cmd != HVMOP_altp2m_set_domain_state) && >>>>>> + !d->arch.altp2m_active ) >>>>> >>>>> Why not using altp2m_active(d) here? >>>>> >>>> >>>> I have already changed that within the next patch version. Thank you. >>>> >>>>> Also this check looks quite racy. What does prevent another CPU to >>>>> disable altp2m at the same time? How the code would behave? >>>>> >>>> >>>> Thank you. I will protect this part with the altp2m_lock. >>> >>> I have noticed that you use the altp2m_lock (it is a spinlock) in >>> multiple places. So you will serialize a lot of code. Is it fine for >>> you? >>> >> >> I would need to move the lock from altp2m_init_by_id to the outside. >> This would not lock much more code as it already does. Apart from that, >> since activating/deactivating altp2m on a specific domain should be used >> very rarely (including the first time when no altp2m structures are >> initialized), it is fine to me. Unless, you would like me to use a >> different lock instead? > > I don't know, it was an open question. There are a couple of places > where you may need to add lock atlp2m_lock (such altp2m_copy_lazy), so > you would serialize all the access. If you care about performance, > then you may want to use a different lock or method of locking (such > as read-write-lock). > Fair enough. I will go through the locks and think about your suggestion, thank you. Best regards, ~Sergej
On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com> wrote: > Hello Sergej, > > > On 01/08/16 18:10, Sergej Proskurin wrote: >> >> This commit moves the altp2m-related code from x86 to ARM. Functions >> that are no yet supported notify the caller or print a BUG message >> stating their absence. >> >> Also, the struct arch_domain is extended with the altp2m_active >> attribute, representing the current altp2m activity configuration of the >> domain. >> >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. >> Removed not used altp2m helper stubs in altp2m.h. >> --- >> xen/arch/arm/hvm.c | 79 >> ++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/altp2m.h | 4 +-- >> xen/include/asm-arm/domain.h | 3 ++ >> 3 files changed, 84 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index d999bde..eb524ae 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -32,6 +32,81 @@ >> >> #include <asm/hypercall.h> >> >> +#include <asm/altp2m.h> >> + >> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >> +{ >> + struct xen_hvm_altp2m_op a; >> + struct domain *d = NULL; >> + int rc = 0; >> + >> + if ( copy_from_guest(&a, arg, 1) ) >> + return -EFAULT; >> + >> + if ( a.pad1 || a.pad2 || >> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >> + (a.cmd < HVMOP_altp2m_get_domain_state) || >> + (a.cmd > HVMOP_altp2m_change_gfn) ) >> + return -EINVAL; >> + >> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >> + rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); >> + >> + if ( d == NULL ) >> + return -ESRCH; >> + >> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >> + (a.cmd != HVMOP_altp2m_set_domain_state) && >> + !d->arch.altp2m_active ) > > > Why not using altp2m_active(d) here? > > Also this check looks quite racy. What does prevent another CPU to disable > altp2m at the same time? How the code would behave? There is a rcu_lock_domain_by_any_id before we get to this check here, so any other CPU looking to disable altp2m would be waiting there for the current op to finish up, so there is no race condition AFAICT. Tamas
Hello Tamas, On 09/08/2016 21:16, Tamas K Lengyel wrote: > On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com> wrote: >> Hello Sergej, >> >> >> On 01/08/16 18:10, Sergej Proskurin wrote: >>> >>> This commit moves the altp2m-related code from x86 to ARM. Functions >>> that are no yet supported notify the caller or print a BUG message >>> stating their absence. >>> >>> Also, the struct arch_domain is extended with the altp2m_active >>> attribute, representing the current altp2m activity configuration of the >>> domain. >>> >>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>> --- >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> --- >>> v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. >>> Removed not used altp2m helper stubs in altp2m.h. >>> --- >>> xen/arch/arm/hvm.c | 79 >>> ++++++++++++++++++++++++++++++++++++++++++++ >>> xen/include/asm-arm/altp2m.h | 4 +-- >>> xen/include/asm-arm/domain.h | 3 ++ >>> 3 files changed, 84 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>> index d999bde..eb524ae 100644 >>> --- a/xen/arch/arm/hvm.c >>> +++ b/xen/arch/arm/hvm.c >>> @@ -32,6 +32,81 @@ >>> >>> #include <asm/hypercall.h> >>> >>> +#include <asm/altp2m.h> >>> + >>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>> +{ >>> + struct xen_hvm_altp2m_op a; >>> + struct domain *d = NULL; >>> + int rc = 0; >>> + >>> + if ( copy_from_guest(&a, arg, 1) ) >>> + return -EFAULT; >>> + >>> + if ( a.pad1 || a.pad2 || >>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >>> + return -EINVAL; >>> + >>> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >>> + rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); >>> + >>> + if ( d == NULL ) >>> + return -ESRCH; >>> + >>> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >>> + (a.cmd != HVMOP_altp2m_set_domain_state) && >>> + !d->arch.altp2m_active ) >> >> >> Why not using altp2m_active(d) here? >> >> Also this check looks quite racy. What does prevent another CPU to disable >> altp2m at the same time? How the code would behave? > > There is a rcu_lock_domain_by_any_id before we get to this check here, > so any other CPU looking to disable altp2m would be waiting there for > the current op to finish up, so there is no race condition AFAICT. No, rcu_lock_domain_by_any_id only prevents the domain to be fully destroyed by "locking" the rcu. It does not prevent multiple concurrent access. You can look at the code if you are not convinced. Regards,
On Aug 10, 2016 03:52, "Julien Grall" <julien.grall@arm.com> wrote: > > Hello Tamas, > > > On 09/08/2016 21:16, Tamas K Lengyel wrote: >> >> On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com> wrote: >>> >>> Hello Sergej, >>> >>> >>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>> >>>> >>>> This commit moves the altp2m-related code from x86 to ARM. Functions >>>> that are no yet supported notify the caller or print a BUG message >>>> stating their absence. >>>> >>>> Also, the struct arch_domain is extended with the altp2m_active >>>> attribute, representing the current altp2m activity configuration of the >>>> domain. >>>> >>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>>> --- >>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>> Cc: Julien Grall <julien.grall@arm.com> >>>> --- >>>> v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. >>>> Removed not used altp2m helper stubs in altp2m.h. >>>> --- >>>> xen/arch/arm/hvm.c | 79 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>> xen/include/asm-arm/altp2m.h | 4 +-- >>>> xen/include/asm-arm/domain.h | 3 ++ >>>> 3 files changed, 84 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>>> index d999bde..eb524ae 100644 >>>> --- a/xen/arch/arm/hvm.c >>>> +++ b/xen/arch/arm/hvm.c >>>> @@ -32,6 +32,81 @@ >>>> >>>> #include <asm/hypercall.h> >>>> >>>> +#include <asm/altp2m.h> >>>> + >>>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>>> +{ >>>> + struct xen_hvm_altp2m_op a; >>>> + struct domain *d = NULL; >>>> + int rc = 0; >>>> + >>>> + if ( copy_from_guest(&a, arg, 1) ) >>>> + return -EFAULT; >>>> + >>>> + if ( a.pad1 || a.pad2 || >>>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >>>> + return -EINVAL; >>>> + >>>> + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >>>> + rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); >>>> + >>>> + if ( d == NULL ) >>>> + return -ESRCH; >>>> + >>>> + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >>>> + (a.cmd != HVMOP_altp2m_set_domain_state) && >>>> + !d->arch.altp2m_active ) >>> >>> >>> >>> Why not using altp2m_active(d) here? >>> >>> Also this check looks quite racy. What does prevent another CPU to disable >>> altp2m at the same time? How the code would behave? >> >> >> There is a rcu_lock_domain_by_any_id before we get to this check here, >> so any other CPU looking to disable altp2m would be waiting there for >> the current op to finish up, so there is no race condition AFAICT. > > > No, rcu_lock_domain_by_any_id only prevents the domain to be fully destroyed by "locking" the rcu. It does not prevent multiple concurrent access. You can look at the code if you are not convinced. > Ah thanks for clarifying. Then indeed there could be concurrency issues if there are multiple tools accessing this interface. Normally that doesn't happen though but probably a good idea to enforce it anyway. Thanks, Tamas
Hello Tamas, On 10/08/2016 16:49, Tamas K Lengyel wrote: > On Aug 10, 2016 03:52, "Julien Grall" <julien.grall@arm.com > <mailto:julien.grall@arm.com>> wrote: >> On 09/08/2016 21:16, Tamas K Lengyel wrote: >>> On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com > <mailto:julien.grall@arm.com>> wrote: >>> There is a rcu_lock_domain_by_any_id before we get to this check here, >>> so any other CPU looking to disable altp2m would be waiting there for >>> the current op to finish up, so there is no race condition AFAICT. >> >> >> No, rcu_lock_domain_by_any_id only prevents the domain to be fully > destroyed by "locking" the rcu. It does not prevent multiple concurrent > access. You can look at the code if you are not convinced. >> > > Ah thanks for clarifying. Then indeed there could be concurrency issues > if there are multiple tools accessing this interface. Normally that > doesn't happen though but probably a good idea to enforce it anyway. Well, you need to think about the worst case scenario when you implement an interface. If you don't lock properly, the state in Xen may be corrupted. For instance Xen may think altp2m is active whilst it is not properly initialized. Regards,
On Aug 11, 2016 02:18, "Julien Grall" <julien.grall@arm.com> wrote: > > Hello Tamas, > > > On 10/08/2016 16:49, Tamas K Lengyel wrote: >> >> On Aug 10, 2016 03:52, "Julien Grall" <julien.grall@arm.com >> <mailto:julien.grall@arm.com>> wrote: >>> >>> On 09/08/2016 21:16, Tamas K Lengyel wrote: >>>> >>>> On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com >> >> <mailto:julien.grall@arm.com>> wrote: >>>> >>>> There is a rcu_lock_domain_by_any_id before we get to this check here, >>>> so any other CPU looking to disable altp2m would be waiting there for >>>> the current op to finish up, so there is no race condition AFAICT. >>> >>> >>> >>> No, rcu_lock_domain_by_any_id only prevents the domain to be fully >> >> destroyed by "locking" the rcu. It does not prevent multiple concurrent >> access. You can look at the code if you are not convinced. >>> >>> >> >> Ah thanks for clarifying. Then indeed there could be concurrency issues >> if there are multiple tools accessing this interface. Normally that >> doesn't happen though but probably a good idea to enforce it anyway. > > > Well, you need to think about the worst case scenario when you implement an interface. If you don't lock properly, the state in Xen may be corrupted. For instance Xen may think altp2m is active whilst it is not properly initialized. > Sure. We largely followed the x86 implementation here and there aren't any hvmops there that do synchronization like that, only the rcu lock is taken. Adding a domain_lock() should be fine though. Tamas
On 11/08/2016 16:41, Tamas K Lengyel wrote: > On Aug 11, 2016 02:18, "Julien Grall" <julien.grall@arm.com > <mailto:julien.grall@arm.com>> wrote: >> >> Hello Tamas, >> >> >> On 10/08/2016 16:49, Tamas K Lengyel wrote: >>> >>> On Aug 10, 2016 03:52, "Julien Grall" <julien.grall@arm.com > <mailto:julien.grall@arm.com> >>> <mailto:julien.grall@arm.com <mailto:julien.grall@arm.com>>> wrote: >>>> >>>> On 09/08/2016 21:16, Tamas K Lengyel wrote: >>>>> >>>>> On Wed, Aug 3, 2016 at 10:54 AM, Julien Grall <julien.grall@arm.com > <mailto:julien.grall@arm.com> >>> >>> <mailto:julien.grall@arm.com <mailto:julien.grall@arm.com>>> wrote: >>>>> >>>>> There is a rcu_lock_domain_by_any_id before we get to this check here, >>>>> so any other CPU looking to disable altp2m would be waiting there for >>>>> the current op to finish up, so there is no race condition AFAICT. >>>> >>>> >>>> >>>> No, rcu_lock_domain_by_any_id only prevents the domain to be fully >>> >>> destroyed by "locking" the rcu. It does not prevent multiple concurrent >>> access. You can look at the code if you are not convinced. >>>> >>>> >>> >>> Ah thanks for clarifying. Then indeed there could be concurrency issues >>> if there are multiple tools accessing this interface. Normally that >>> doesn't happen though but probably a good idea to enforce it anyway. >> >> >> Well, you need to think about the worst case scenario when you > implement an interface. If you don't lock properly, the state in Xen may > be corrupted. For instance Xen may think altp2m is active whilst it is > not properly initialized. >> > > Sure. We largely followed the x86 implementation here and there aren't > any hvmops there that do synchronization like that, only the rcu lock is > taken. Adding a domain_lock() should be fine though. I would be curious to know why the x86 implementation does not need this lock. Cheers,
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index d999bde..eb524ae 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -32,6 +32,81 @@ #include <asm/hypercall.h> +#include <asm/altp2m.h> + +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) +{ + struct xen_hvm_altp2m_op a; + struct domain *d = NULL; + int rc = 0; + + if ( copy_from_guest(&a, arg, 1) ) + return -EFAULT; + + if ( a.pad1 || a.pad2 || + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || + (a.cmd < HVMOP_altp2m_get_domain_state) || + (a.cmd > HVMOP_altp2m_change_gfn) ) + return -EINVAL; + + d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? + rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); + + if ( d == NULL ) + return -ESRCH; + + if ( (a.cmd != HVMOP_altp2m_get_domain_state) && + (a.cmd != HVMOP_altp2m_set_domain_state) && + !d->arch.altp2m_active ) + { + rc = -EOPNOTSUPP; + goto out; + } + + if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) ) + goto out; + + switch ( a.cmd ) + { + case HVMOP_altp2m_get_domain_state: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_set_domain_state: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_vcpu_enable_notify: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_create_p2m: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_destroy_p2m: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_switch_p2m: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_set_mem_access: + rc = -EOPNOTSUPP; + break; + + case HVMOP_altp2m_change_gfn: + rc = -EOPNOTSUPP; + break; + } + +out: + rcu_unlock_domain(d); + + return rc; +} + long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) { long rc = 0; @@ -80,6 +155,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) rc = -EINVAL; break; + case HVMOP_altp2m: + rc = do_altp2m_op(arg); + break; + default: { gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op); diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h index a87747a..0711796 100644 --- a/xen/include/asm-arm/altp2m.h +++ b/xen/include/asm-arm/altp2m.h @@ -2,6 +2,7 @@ * Alternate p2m * * Copyright (c) 2014, Intel Corporation. + * Copyright (c) 2016, Sergej Proskurin <proskurin@sec.in.tum.de>. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -24,8 +25,7 @@ /* Alternate p2m on/off per domain */ static inline bool_t altp2m_active(const struct domain *d) { - /* Not implemented on ARM. */ - return 0; + return d->arch.altp2m_active; } /* Alternate p2m VCPU */ diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 9452fcd..cc4bda0 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -126,6 +126,9 @@ struct arch_domain paddr_t efi_acpi_gpa; paddr_t efi_acpi_len; #endif + + /* altp2m: allow multiple copies of host p2m */ + bool_t altp2m_active; } __cacheline_aligned; struct arch_vcpu
This commit moves the altp2m-related code from x86 to ARM. Functions that are no yet supported notify the caller or print a BUG message stating their absence. Also, the struct arch_domain is extended with the altp2m_active attribute, representing the current altp2m activity configuration of the domain. Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M. Removed not used altp2m helper stubs in altp2m.h. --- xen/arch/arm/hvm.c | 79 ++++++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/altp2m.h | 4 +-- xen/include/asm-arm/domain.h | 3 ++ 3 files changed, 84 insertions(+), 2 deletions(-)