| Message ID | 20191128093828.8462-1-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed | expand |
On 28.11.2019 10:38, Paul Durrant wrote: > From: Julien Grall <jgrall@amazon.com> > > A guest will setup a shared page with the hypervisor for each vCPU via > XENPMU_init. The page will then get mapped in the hypervisor and only > released when XENPMU_finish is called. > > This means that if the guest fails to invoke XENPMU_finish, e.g if it is > destroyed rather than cleanly shut down, the page will stay mapped in the > hypervisor. One of the consequences is the domain can never be fully > destroyed as a page reference is still held. > > As Xen should never rely on the guest to correctly clean-up any > allocation in the hypervisor, we should also unmap such pages during the > domain destruction if there are any left. > > We can re-use the same logic as in pvpmu_finish(). To avoid > duplication, move the logic in a new function that can also be called > from vpmu_destroy(). > > NOTE: - The call to vpmu_destroy() must also be moved from > arch_vcpu_destroy() into domain_relinquish_resources() such that > the reference on the mapped page does not prevent domain_destroy() > (which calls arch_vcpu_destroy()) from being called. > - Whilst it appears that vpmu_arch_destroy() is idempotent it is > by no means obvious. Hence make sure the VPMU_CONTEXT_ALLOCATED > flag is cleared at the end of vpmu_arch_destroy(). > - This is not an XSA because vPMU is not security supported (see > XSA-163). > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > > v2: > - Re-word commit comment slightly > - Re-enforce idempotency of vmpu_arch_destroy() > - Move invocation of vpmu_destroy() earlier in > domain_relinquish_resources() What about v3? > --- a/xen/arch/x86/cpu/vpmu.c > +++ b/xen/arch/x86/cpu/vpmu.c > @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > } > + > + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); > } Boris, to be on the safe side - are you in agreement with this change, now that the setting of the flag is being left untouched? Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 28 November 2019 10:23 > To: Durrant, Paul <pdurrant@amazon.com>; Boris Ostrovsky > <boris.ostrovsky@oracle.com> > Cc: xen-devel@lists.xenproject.org; Grall, Julien <jgrall@amazon.com>; > Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné > <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian > <kevin.tian@intel.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v3] xen/x86: vpmu: Unmap per-vCPU PMU page when the > domain is destroyed > > On 28.11.2019 10:38, Paul Durrant wrote: > > From: Julien Grall <jgrall@amazon.com> > > > > A guest will setup a shared page with the hypervisor for each vCPU via > > XENPMU_init. The page will then get mapped in the hypervisor and only > > released when XENPMU_finish is called. > > > > This means that if the guest fails to invoke XENPMU_finish, e.g if it is > > destroyed rather than cleanly shut down, the page will stay mapped in > the > > hypervisor. One of the consequences is the domain can never be fully > > destroyed as a page reference is still held. > > > > As Xen should never rely on the guest to correctly clean-up any > > allocation in the hypervisor, we should also unmap such pages during the > > domain destruction if there are any left. > > > > We can re-use the same logic as in pvpmu_finish(). To avoid > > duplication, move the logic in a new function that can also be called > > from vpmu_destroy(). > > > > NOTE: - The call to vpmu_destroy() must also be moved from > > arch_vcpu_destroy() into domain_relinquish_resources() such that > > the reference on the mapped page does not prevent > domain_destroy() > > (which calls arch_vcpu_destroy()) from being called. > > - Whilst it appears that vpmu_arch_destroy() is idempotent it is > > by no means obvious. Hence make sure the VPMU_CONTEXT_ALLOCATED > > flag is cleared at the end of vpmu_arch_destroy(). > > - This is not an XSA because vPMU is not security supported (see > > XSA-163). > > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Wei Liu <wl@xen.org> > > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > Cc: Kevin Tian <kevin.tian@intel.com> > > > > v2: > > - Re-word commit comment slightly > > - Re-enforce idempotency of vmpu_arch_destroy() > > - Move invocation of vpmu_destroy() earlier in > > domain_relinquish_resources() > > What about v3? Oh, sorry: v3: - Add comment regarding XSA-163 - Revert changes setting VPMU_CONTEXT_ALLOCATED in common code Paul > > > --- a/xen/arch/x86/cpu/vpmu.c > > +++ b/xen/arch/x86/cpu/vpmu.c > > @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) > > > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > > } > > + > > + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); > > } > > Boris, to be on the safe side - are you in agreement with this > change, now that the setting of the flag is being left untouched? > > Jan
On 11/28/19 5:23 AM, Jan Beulich wrote: > On 28.11.2019 10:38, Paul Durrant wrote: > >> --- a/xen/arch/x86/cpu/vpmu.c >> +++ b/xen/arch/x86/cpu/vpmu.c >> @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >> >> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >> } >> + >> + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >> } > Boris, to be on the safe side - are you in agreement with this > change, now that the setting of the flag is being left untouched? Yes, this is fine. (I probably would clear it in arch_vpmu_destroy op since it is set in arch-specific code but either way works) -boris
On 28.11.2019 10:38, Paul Durrant wrote: > From: Julien Grall <jgrall@amazon.com> > > A guest will setup a shared page with the hypervisor for each vCPU via > XENPMU_init. The page will then get mapped in the hypervisor and only > released when XENPMU_finish is called. > > This means that if the guest fails to invoke XENPMU_finish, e.g if it is > destroyed rather than cleanly shut down, the page will stay mapped in the > hypervisor. One of the consequences is the domain can never be fully > destroyed as a page reference is still held. > > As Xen should never rely on the guest to correctly clean-up any > allocation in the hypervisor, we should also unmap such pages during the > domain destruction if there are any left. > > We can re-use the same logic as in pvpmu_finish(). To avoid > duplication, move the logic in a new function that can also be called > from vpmu_destroy(). > > NOTE: - The call to vpmu_destroy() must also be moved from > arch_vcpu_destroy() into domain_relinquish_resources() such that > the reference on the mapped page does not prevent domain_destroy() > (which calls arch_vcpu_destroy()) from being called. > - Whilst it appears that vpmu_arch_destroy() is idempotent it is > by no means obvious. Hence make sure the VPMU_CONTEXT_ALLOCATED > flag is cleared at the end of vpmu_arch_destroy(). > - This is not an XSA because vPMU is not security supported (see > XSA-163). > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Paul Durrant <pdurrant@amazon.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Hi Boris, On 28/11/2019 21:50, Boris Ostrovsky wrote: > On 11/28/19 5:23 AM, Jan Beulich wrote: >> On 28.11.2019 10:38, Paul Durrant wrote: >> >>> --- a/xen/arch/x86/cpu/vpmu.c >>> +++ b/xen/arch/x86/cpu/vpmu.c >>> @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >>> >>> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >>> } >>> + >>> + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >>> } >> Boris, to be on the safe side - are you in agreement with this >> change, now that the setting of the flag is being left untouched? > > Yes, this is fine. Can we take this as an ACK? > > (I probably would clear it in arch_vpmu_destroy op since it is set in > arch-specific code but either way works) > > -boris > Cheers,
> On Dec 4, 2019, at 10:55 AM, Julien Grall <jgrall@amazon.com> wrote: > > Hi Boris, > > On 28/11/2019 21:50, Boris Ostrovsky wrote: >> On 11/28/19 5:23 AM, Jan Beulich wrote: >>> On 28.11.2019 10:38, Paul Durrant wrote: >>> >>>> --- a/xen/arch/x86/cpu/vpmu.c >>>> +++ b/xen/arch/x86/cpu/vpmu.c >>>> @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >>>> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >>>> } >>>> + >>>> + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >>>> } >>> Boris, to be on the safe side - are you in agreement with this >>> change, now that the setting of the flag is being left untouched? >> Yes, this is fine. > > Can we take this as an ACK? Yes, of course. -boris > >> (I probably would clear it in arch_vpmu_destroy op since it is set in >> arch-specific code but either way works) >> -boris > > Cheers,
On 04/12/2019 15:59, Boris Ostrovsky wrote: > > >> On Dec 4, 2019, at 10:55 AM, Julien Grall <jgrall@amazon.com> wrote: >> >> Hi Boris, >> >> On 28/11/2019 21:50, Boris Ostrovsky wrote: >>> On 11/28/19 5:23 AM, Jan Beulich wrote: >>>> On 28.11.2019 10:38, Paul Durrant wrote: >>>> >>>>> --- a/xen/arch/x86/cpu/vpmu.c >>>>> +++ b/xen/arch/x86/cpu/vpmu.c >>>>> @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >>>>> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >>>>> } >>>>> + >>>>> + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >>>>> } >>>> Boris, to be on the safe side - are you in agreement with this >>>> change, now that the setting of the flag is being left untouched? >>> Yes, this is fine. >> >> Can we take this as an ACK? > > > Yes, of course. Thank you! @Andrew, @Jan: this is x86 code, but I am happy to commit it if you prefer. Cheers,
On 04.12.2019 17:02, Julien Grall wrote: > > > On 04/12/2019 15:59, Boris Ostrovsky wrote: >> >> >>> On Dec 4, 2019, at 10:55 AM, Julien Grall <jgrall@amazon.com> wrote: >>> >>> Hi Boris, >>> >>> On 28/11/2019 21:50, Boris Ostrovsky wrote: >>>> On 11/28/19 5:23 AM, Jan Beulich wrote: >>>>> On 28.11.2019 10:38, Paul Durrant wrote: >>>>> >>>>>> --- a/xen/arch/x86/cpu/vpmu.c >>>>>> +++ b/xen/arch/x86/cpu/vpmu.c >>>>>> @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >>>>>> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >>>>>> } >>>>>> + >>>>>> + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >>>>>> } >>>>> Boris, to be on the safe side - are you in agreement with this >>>>> change, now that the setting of the flag is being left untouched? >>>> Yes, this is fine. >>> >>> Can we take this as an ACK? >> >> >> Yes, of course. > > Thank you! > > @Andrew, @Jan: this is x86 code, but I am happy to commit it if you prefer. It has been committed already. Jan
On 04/12/2019 16:08, Jan Beulich wrote: > On 04.12.2019 17:02, Julien Grall wrote: >> >> >> On 04/12/2019 15:59, Boris Ostrovsky wrote: >>> >>> >>>> On Dec 4, 2019, at 10:55 AM, Julien Grall <jgrall@amazon.com> wrote: >>>> >>>> Hi Boris, >>>> >>>> On 28/11/2019 21:50, Boris Ostrovsky wrote: >>>>> On 11/28/19 5:23 AM, Jan Beulich wrote: >>>>>> On 28.11.2019 10:38, Paul Durrant wrote: >>>>>> >>>>>>> --- a/xen/arch/x86/cpu/vpmu.c >>>>>>> +++ b/xen/arch/x86/cpu/vpmu.c >>>>>>> @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >>>>>>> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >>>>>>> } >>>>>>> + >>>>>>> + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >>>>>>> } >>>>>> Boris, to be on the safe side - are you in agreement with this >>>>>> change, now that the setting of the flag is being left untouched? >>>>> Yes, this is fine. >>>> >>>> Can we take this as an ACK? >>> >>> >>> Yes, of course. >> >> Thank you! >> >> @Andrew, @Jan: this is x86 code, but I am happy to commit it if you prefer. > > It has been committed already. Oh, I didn't spot it in git log. Sorry for the noise. Cheers,
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c index f397183ec3..792953e7cb 100644 --- a/xen/arch/x86/cpu/vpmu.c +++ b/xen/arch/x86/cpu/vpmu.c @@ -576,11 +576,36 @@ static void vpmu_arch_destroy(struct vcpu *v) vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); } + + vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); } -void vpmu_destroy(struct vcpu *v) +static void vpmu_cleanup(struct vcpu *v) { + struct vpmu_struct *vpmu = vcpu_vpmu(v); + mfn_t mfn; + void *xenpmu_data; + + spin_lock(&vpmu->vpmu_lock); + vpmu_arch_destroy(v); + xenpmu_data = vpmu->xenpmu_data; + vpmu->xenpmu_data = NULL; + + spin_unlock(&vpmu->vpmu_lock); + + if ( xenpmu_data ) + { + mfn = domain_page_map_to_mfn(xenpmu_data); + ASSERT(mfn_valid(mfn)); + unmap_domain_page_global(xenpmu_data); + put_page_and_type(mfn_to_page(mfn)); + } +} + +void vpmu_destroy(struct vcpu *v) +{ + vpmu_cleanup(v); put_vpmu(v); } @@ -639,9 +664,6 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t *params) static void pvpmu_finish(struct domain *d, xen_pmu_params_t *params) { struct vcpu *v; - struct vpmu_struct *vpmu; - mfn_t mfn; - void *xenpmu_data; if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] == NULL) ) return; @@ -650,22 +672,7 @@ static void pvpmu_finish(struct domain *d, xen_pmu_params_t *params) if ( v != current ) vcpu_pause(v); - vpmu = vcpu_vpmu(v); - spin_lock(&vpmu->vpmu_lock); - - vpmu_arch_destroy(v); - xenpmu_data = vpmu->xenpmu_data; - vpmu->xenpmu_data = NULL; - - spin_unlock(&vpmu->vpmu_lock); - - if ( xenpmu_data ) - { - mfn = domain_page_map_to_mfn(xenpmu_data); - ASSERT(mfn_valid(mfn)); - unmap_domain_page_global(xenpmu_data); - put_page_and_type(mfn_to_page(mfn)); - } + vpmu_cleanup(v); if ( v != current ) vcpu_unpause(v); diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index f1dd86e12e..f5c0c378ef 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -454,9 +454,6 @@ void arch_vcpu_destroy(struct vcpu *v) xfree(v->arch.msrs); v->arch.msrs = NULL; - if ( !is_idle_domain(v->domain) ) - vpmu_destroy(v); - if ( is_hvm_vcpu(v) ) hvm_vcpu_destroy(v); else @@ -2136,12 +2133,17 @@ int domain_relinquish_resources(struct domain *d) PROGRESS(vcpu_pagetables): - /* Drop the in-use references to page-table bases. */ + /* + * Drop the in-use references to page-table bases and clean + * up vPMU instances. + */ for_each_vcpu ( d, v ) { ret = vcpu_destroy_pagetables(v); if ( ret ) return ret; + + vpmu_destroy(v); } if ( altp2m_active(d) )