1: vLAPIC: don't leak regs page from vlapic_init() upon error 2: fix resource leaks on arch_vcpu_create() error path 3: vLAPIC: vlapic_init() runs only once for a vCPU Jan
Fixes: 8a981e0bf25e ("Make map_domain_page_global fail") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1625,6 +1625,7 @@ int vlapic_init(struct vcpu *v) vlapic->regs = __map_domain_page_global(vlapic->regs_page); if ( vlapic->regs == NULL ) { + free_domheap_page(vlapic->regs_page); dprintk(XENLOG_ERR, "map vlapic regs error: %d/%d\n", v->domain->domain_id, v->vcpu_id); return -ENOMEM;
On 02/10/2020 11:30, Jan Beulich wrote: > Fixes: 8a981e0bf25e ("Make map_domain_page_global fail") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
{hvm,pv}_vcpu_initialise() have always been meant to be the final possible source of errors in arch_vcpu_create(), hence not requiring any unrolling of what they've done on the error path. (Of course this may change once the various involved paths all have become idempotent.) But even beyond this aspect I think it is more logical to do policy initialization ahead of the calling of these two functions, as they may in principle want to access it. Fixes: 4187f79dc718 ("x86/msr: introduce struct msr_vcpu_policy") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -569,6 +569,9 @@ int arch_vcpu_create(struct vcpu *v) vmce_init_vcpu(v); arch_vcpu_regs_init(v); + + if ( (rc = init_vcpu_msr_policy(v)) ) + goto fail; } else if ( (rc = xstate_alloc_save_area(v)) != 0 ) return rc; @@ -594,9 +597,6 @@ int arch_vcpu_create(struct vcpu *v) { vpmu_initialise(v); - if ( (rc = init_vcpu_msr_policy(v)) ) - goto fail; - cpuid_policy_updated(v); }
On 02/10/2020 11:30, Jan Beulich wrote: > {hvm,pv}_vcpu_initialise() have always been meant to be the final > possible source of errors in arch_vcpu_create(), hence not requiring > any unrolling of what they've done on the error path. (Of course this > may change once the various involved paths all have become idempotent.) I'd agree that the way the code was previously laid out expected {hvm,pv}_vcpu_initialise() to be the final failing option. I don't think "has always meant to be" is reasonable, because where is the code comment explaining this design choice? The idempotent plans will definitely be removing this misbehaviour, and the memory leaks it causes. > But even beyond this aspect I think it is more logical to do policy > initialization ahead of the calling of these two functions, as they may > in principle want to access it. Not these MSRs. They're currently a block of zeroes, and while that will eventually change, it will still be a bunch of MSRs in their RESET state. The interesting MSRs are the domain ones, not the vCPU ones. > > Fixes: 4187f79dc718 ("x86/msr: introduce struct msr_vcpu_policy") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> although I'd prefer some adjustment to the commit message along the indicated lines.
On 02.10.2020 13:13, Andrew Cooper wrote: > On 02/10/2020 11:30, Jan Beulich wrote: >> {hvm,pv}_vcpu_initialise() have always been meant to be the final >> possible source of errors in arch_vcpu_create(), hence not requiring >> any unrolling of what they've done on the error path. (Of course this >> may change once the various involved paths all have become idempotent.) > > I'd agree that the way the code was previously laid out expected > {hvm,pv}_vcpu_initialise() to be the final failing option. > > I don't think "has always meant to be" is reasonable, because where is > the code comment explaining this design choice? It's probably more a "happened to be that way and then it was easiest to keep it like this", but I recall the behavior having been the subject of discussions, with the outcome that it's at least "kind of" intended. Would adding "kind of" make things look better to you? >> But even beyond this aspect I think it is more logical to do policy >> initialization ahead of the calling of these two functions, as they may >> in principle want to access it. > > Not these MSRs. They're currently a block of zeroes, and while that > will eventually change, it will still be a bunch of MSRs in their RESET > state. > > The interesting MSRs are the domain ones, not the vCPU ones. If you had said "The more interesting ...", I'd have agreed. What I was thinking of as possible uses (be it reading or writing) is e.g. reset state that may depend on certain further properties. Furthermore I was thinking of code paths that vCPU initialization simply may re-use, and which expect the policy to at least be available, irrespective of the individual MSRs' values still being their reset ones. >> Fixes: 4187f79dc718 ("x86/msr: introduce struct msr_vcpu_policy") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> although I'd > prefer some adjustment to the commit message along the indicated lines. Thanks. As far as adjustments go, I don't really see how to better reflect what you want, considering my replies above. If you have any hints ... (I'll hold off committing this for a little while, but I think I'd like to put it in before I leave for weekend and vacation.) Jan
Hence there's no need to guard allocation / mapping by checks whether the same action has been done before. I assume this was a transient change which should have been undone before 509529e99148 ("x86 hvm: Xen interface and implementation for virtual S3") got committed. While touching this code, switch dprintk()-s to use %pv. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1610,27 +1610,21 @@ int vlapic_init(struct vcpu *v) vlapic->pt.source = PTSRC_lapic; - if (vlapic->regs_page == NULL) + vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner); + if ( !vlapic->regs_page ) { - vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner); - if ( vlapic->regs_page == NULL ) - { - dprintk(XENLOG_ERR, "alloc vlapic regs error: %d/%d\n", - v->domain->domain_id, v->vcpu_id); - return -ENOMEM; - } + dprintk(XENLOG_ERR, "%pv: alloc vlapic regs error\n", v); + return -ENOMEM; } - if (vlapic->regs == NULL) + + vlapic->regs = __map_domain_page_global(vlapic->regs_page); + if ( vlapic->regs == NULL ) { - vlapic->regs = __map_domain_page_global(vlapic->regs_page); - if ( vlapic->regs == NULL ) - { - free_domheap_page(vlapic->regs_page); - dprintk(XENLOG_ERR, "map vlapic regs error: %d/%d\n", - v->domain->domain_id, v->vcpu_id); - return -ENOMEM; - } + free_domheap_page(vlapic->regs_page); + dprintk(XENLOG_ERR, "%pv: map vlapic regs error\n", v); + return -ENOMEM; } + clear_page(vlapic->regs); vlapic_reset(vlapic);
On 02/10/2020 11:31, Jan Beulich wrote: > Hence there's no need to guard allocation / mapping by checks whether > the same action has been done before. I assume this was a transient > change which should have been undone before 509529e99148 ("x86 hvm: Xen > interface and implementation for virtual S3") got committed. > > While touching this code, switch dprintk()-s to use %pv. Logging ENOMEM, especially without actually saying ENOMEM, is quite pointless. > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with the printk()s dropped.
On 02.10.2020 13:19, Andrew Cooper wrote: > On 02/10/2020 11:31, Jan Beulich wrote: >> Hence there's no need to guard allocation / mapping by checks whether >> the same action has been done before. I assume this was a transient >> change which should have been undone before 509529e99148 ("x86 hvm: Xen >> interface and implementation for virtual S3") got committed. >> >> While touching this code, switch dprintk()-s to use %pv. > > Logging ENOMEM, especially without actually saying ENOMEM, is quite > pointless. > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with > the printk()s dropped. Thanks, and sure - I'll be happy to drop them. Just didn't want to make more of a change than needed, and them being dprintk()-s didn't make them look all that awful. Jan
© 2016 - 2024 Red Hat, Inc.