| Message ID | 20160704114605.10086-9-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, On 04/07/16 12:45, Sergej Proskurin wrote: > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/hvm.c | 2 +- > xen/arch/arm/p2m.c | 32 ++++++++++++++++++++++++++++++++ > xen/include/asm-arm/p2m.h | 3 +++ > 3 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 005d7c6..f4ec5cf 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -145,7 +145,7 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) > break; > > case HVMOP_altp2m_destroy_p2m: > - rc = -EOPNOTSUPP; > + rc = p2m_destroy_altp2m_by_id(d, a.u.view.view); > break; > > case HVMOP_altp2m_switch_p2m: > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 6c41b98..f82f1ea 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -2200,6 +2200,38 @@ void p2m_flush_altp2m(struct domain *d) > altp2m_unlock(d); > } > > +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) > +{ > + struct p2m_domain *p2m; > + int rc = -EBUSY; > + > + if ( !idx || idx >= MAX_ALTP2M ) Can you please add a comment to explain why the altp2m at index 0 cannot be destroyed. > + return rc; > + > + domain_pause_except_self(d); > + > + altp2m_lock(d); > + > + if ( d->arch.altp2m_vttbr[idx] != INVALID_MFN ) > + { > + p2m = d->arch.altp2m_p2m[idx]; > + > + if ( !_atomic_read(p2m->active_vcpus) ) > + { > + p2m_flush_table(p2m); > + flush_tlb(); Can you explain this call? flush_tlb is flushing TLBs for the current VMID only. However, d may not be equal to current when calling from HVM op. > + d->arch.altp2m_vttbr[idx] = INVALID_MFN; > + rc = 0; > + } > + } > + > + altp2m_unlock(d); > + > + domain_unpause_except_self(d); > + > + return rc; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index c51532a..255a282 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -140,6 +140,9 @@ 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); > > +/* Make a specific alternate p2m invalid */ > +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); > + > #define p2m_is_foreign(_t) ((_t) == p2m_map_foreign) > #define p2m_is_ram(_t) ((_t) == p2m_ram_rw || (_t) == p2m_ram_ro) > > Regards,
Hi Julien, On 07/04/2016 06:32 PM, Julien Grall wrote: > Hello Sergej, > > On 04/07/16 12:45, Sergej Proskurin wrote: >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/hvm.c | 2 +- >> xen/arch/arm/p2m.c | 32 ++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/p2m.h | 3 +++ >> 3 files changed, 36 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index 005d7c6..f4ec5cf 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -145,7 +145,7 @@ static int >> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >> break; >> >> case HVMOP_altp2m_destroy_p2m: >> - rc = -EOPNOTSUPP; >> + rc = p2m_destroy_altp2m_by_id(d, a.u.view.view); >> break; >> >> case HVMOP_altp2m_switch_p2m: >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 6c41b98..f82f1ea 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -2200,6 +2200,38 @@ void p2m_flush_altp2m(struct domain *d) >> altp2m_unlock(d); >> } >> >> +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) >> +{ >> + struct p2m_domain *p2m; >> + int rc = -EBUSY; >> + >> + if ( !idx || idx >= MAX_ALTP2M ) > > Can you please add a comment to explain why the altp2m at index 0 > cannot be destroyed. > This has been adopted from the x86 implementation. The altp2m[0] is considered as hostp2m and is used as a safe harbor to which you can switch as long as altp2m is active. After deactivating altp2m, the system switches back to the original hostp2m view.That is, altp2m[0] should only be destroyed/flushed/freed, when altp2m is deactivated. >> + return rc; >> + >> + domain_pause_except_self(d); >> + >> + altp2m_lock(d); >> + >> + if ( d->arch.altp2m_vttbr[idx] != INVALID_MFN ) >> + { >> + p2m = d->arch.altp2m_p2m[idx]; >> + >> + if ( !_atomic_read(p2m->active_vcpus) ) >> + { >> + p2m_flush_table(p2m); >> + flush_tlb(); > > Can you explain this call? flush_tlb is flushing TLBs for the current > VMID only. However, d may not be equal to current when calling from > HVM op. > You are correct. Please correct me if I am wrong, but I believe we even do not need flushing at this point any more (see my response in patch #06).Thank you. >> + d->arch.altp2m_vttbr[idx] = INVALID_MFN; >> + rc = 0; >> + } >> + } >> + >> + altp2m_unlock(d); >> + >> + domain_unpause_except_self(d); >> + >> + return rc; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index c51532a..255a282 100644 >> --- a/xen/include/asm-arm/p2m.h >> +++ b/xen/include/asm-arm/p2m.h >> @@ -140,6 +140,9 @@ 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); >> >> +/* Make a specific alternate p2m invalid */ >> +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); >> + >> #define p2m_is_foreign(_t) ((_t) == p2m_map_foreign) >> #define p2m_is_ram(_t) ((_t) == p2m_ram_rw || (_t) == p2m_ram_ro) >> >> > > Regards, > ~Sergej
On 05/07/16 12:37, Sergej Proskurin wrote: > On 07/04/2016 06:32 PM, Julien Grall wrote: >> Hello Sergej, >> >> On 04/07/16 12:45, Sergej Proskurin wrote: >>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>> --- >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> --- >>> xen/arch/arm/hvm.c | 2 +- >>> xen/arch/arm/p2m.c | 32 ++++++++++++++++++++++++++++++++ >>> xen/include/asm-arm/p2m.h | 3 +++ >>> 3 files changed, 36 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>> index 005d7c6..f4ec5cf 100644 >>> --- a/xen/arch/arm/hvm.c >>> +++ b/xen/arch/arm/hvm.c >>> @@ -145,7 +145,7 @@ static int >>> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>> break; >>> >>> case HVMOP_altp2m_destroy_p2m: >>> - rc = -EOPNOTSUPP; >>> + rc = p2m_destroy_altp2m_by_id(d, a.u.view.view); >>> break; >>> >>> case HVMOP_altp2m_switch_p2m: >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index 6c41b98..f82f1ea 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -2200,6 +2200,38 @@ void p2m_flush_altp2m(struct domain *d) >>> altp2m_unlock(d); >>> } >>> >>> +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) >>> +{ >>> + struct p2m_domain *p2m; >>> + int rc = -EBUSY; >>> + >>> + if ( !idx || idx >= MAX_ALTP2M ) >> >> Can you please add a comment to explain why the altp2m at index 0 >> cannot be destroyed. >> > > This has been adopted from the x86 implementation. The altp2m[0] is > considered as hostp2m and is used as a safe harbor to which you can > switch as long as altp2m is active. After deactivating altp2m, the > system switches back to the original hostp2m view.That is, altp2m[0] > should only be destroyed/flushed/freed, when altp2m is deactivated. Please add a comment in the code to explain it. Regards,
Hi Julien, On 07/05/2016 01:48 PM, Julien Grall wrote: > > > On 05/07/16 12:37, Sergej Proskurin wrote: >> On 07/04/2016 06:32 PM, Julien Grall wrote: >>> Hello Sergej, >>> >>> On 04/07/16 12:45, Sergej Proskurin wrote: >>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >>>> --- >>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>> Cc: Julien Grall <julien.grall@arm.com> >>>> --- >>>> xen/arch/arm/hvm.c | 2 +- >>>> xen/arch/arm/p2m.c | 32 ++++++++++++++++++++++++++++++++ >>>> xen/include/asm-arm/p2m.h | 3 +++ >>>> 3 files changed, 36 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >>>> index 005d7c6..f4ec5cf 100644 >>>> --- a/xen/arch/arm/hvm.c >>>> +++ b/xen/arch/arm/hvm.c >>>> @@ -145,7 +145,7 @@ static int >>>> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >>>> break; >>>> >>>> case HVMOP_altp2m_destroy_p2m: >>>> - rc = -EOPNOTSUPP; >>>> + rc = p2m_destroy_altp2m_by_id(d, a.u.view.view); >>>> break; >>>> >>>> case HVMOP_altp2m_switch_p2m: >>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>> index 6c41b98..f82f1ea 100644 >>>> --- a/xen/arch/arm/p2m.c >>>> +++ b/xen/arch/arm/p2m.c >>>> @@ -2200,6 +2200,38 @@ void p2m_flush_altp2m(struct domain *d) >>>> altp2m_unlock(d); >>>> } >>>> >>>> +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) >>>> +{ >>>> + struct p2m_domain *p2m; >>>> + int rc = -EBUSY; >>>> + >>>> + if ( !idx || idx >= MAX_ALTP2M ) >>> >>> Can you please add a comment to explain why the altp2m at index 0 >>> cannot be destroyed. >>> >> >> This has been adopted from the x86 implementation. The altp2m[0] is >> considered as hostp2m and is used as a safe harbor to which you can >> switch as long as altp2m is active. After deactivating altp2m, the >> system switches back to the original hostp2m view.That is, altp2m[0] >> should only be destroyed/flushed/freed, when altp2m is deactivated. > > Please add a comment in the code to explain it. > Ok, I will add an appropriate comment, thank you. Cheers, ~Sergej
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 005d7c6..f4ec5cf 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -145,7 +145,7 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) break; case HVMOP_altp2m_destroy_p2m: - rc = -EOPNOTSUPP; + rc = p2m_destroy_altp2m_by_id(d, a.u.view.view); break; case HVMOP_altp2m_switch_p2m: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 6c41b98..f82f1ea 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -2200,6 +2200,38 @@ void p2m_flush_altp2m(struct domain *d) altp2m_unlock(d); } +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) +{ + struct p2m_domain *p2m; + int rc = -EBUSY; + + if ( !idx || idx >= MAX_ALTP2M ) + return rc; + + domain_pause_except_self(d); + + altp2m_lock(d); + + if ( d->arch.altp2m_vttbr[idx] != INVALID_MFN ) + { + p2m = d->arch.altp2m_p2m[idx]; + + if ( !_atomic_read(p2m->active_vcpus) ) + { + p2m_flush_table(p2m); + flush_tlb(); + d->arch.altp2m_vttbr[idx] = INVALID_MFN; + rc = 0; + } + } + + altp2m_unlock(d); + + domain_unpause_except_self(d); + + return rc; +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index c51532a..255a282 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -140,6 +140,9 @@ 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); +/* Make a specific alternate p2m invalid */ +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); + #define p2m_is_foreign(_t) ((_t) == p2m_map_foreign) #define p2m_is_ram(_t) ((_t) == p2m_ram_rw || (_t) == p2m_ram_ro)
Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/hvm.c | 2 +- xen/arch/arm/p2m.c | 32 ++++++++++++++++++++++++++++++++ xen/include/asm-arm/p2m.h | 3 +++ 3 files changed, 36 insertions(+), 1 deletion(-)