Re: [Xen-devel] [PATCH] x86/HVM: correct an inverted check in hvm_load()
On 02/08/18 07:02, Jan Beulich wrote: On 01.08.18 at 18:08, wrote: >> On 01/08/18 16:36, Jan Beulich wrote: >>> Clearly we want to put a vCPU to sleep if it is _not_ already down. >>> >>> Signed-off-by: Jan Beulich >>> --- >>> TBD: Since the flaw apparently never mattered, I imply that the function >>> is never called with any vCPU up. Hence an alternative might be to >>> simply return an error if a vCPU doesn't have VPF_down set. >> With Remus/COLO, we should hit this path with an up vCPU on every >> iteration after the first. Given this bug, I'm struggling to see how it >> works at all. > Well, that would rule out the alternative suggestion, but since > this is neither an ack nor a nak - what do you suggest we do? Apologies for completely missing this. The fix is obvious, so Reviewed-by: Andrew Cooper I don't think we can instead return an error, because of the Remus/COLO usecase, but I expect someone is going to have some debugging to do... ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/HVM: correct an inverted check in hvm_load()
>>> On 01.08.18 at 18:08, wrote: > On 01/08/18 16:36, Jan Beulich wrote: >> Clearly we want to put a vCPU to sleep if it is _not_ already down. >> >> Signed-off-by: Jan Beulich >> --- >> TBD: Since the flaw apparently never mattered, I imply that the function >> is never called with any vCPU up. Hence an alternative might be to >> simply return an error if a vCPU doesn't have VPF_down set. > > With Remus/COLO, we should hit this path with an up vCPU on every > iteration after the first. Given this bug, I'm struggling to see how it > works at all. Well, that would rule out the alternative suggestion, but since this is neither an ack nor a nak - what do you suggest we do? Jan >> --- a/xen/arch/x86/hvm/save.c >> +++ b/xen/arch/x86/hvm/save.c >> @@ -268,7 +268,7 @@ int hvm_load(struct domain *d, hvm_domai >> >> /* Down all the vcpus: we only re-enable the ones that had state saved. >> */ >> for_each_vcpu(d, v) >> -if ( test_and_set_bit(_VPF_down, &v->pause_flags) ) >> +if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) >> vcpu_sleep_nosync(v); >> >> for ( ; ; ) >> >> ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/HVM: correct an inverted check in hvm_load()
On 01/08/18 16:36, Jan Beulich wrote: > Clearly we want to put a vCPU to sleep if it is _not_ already down. > > Signed-off-by: Jan Beulich > --- > TBD: Since the flaw apparently never mattered, I imply that the function > is never called with any vCPU up. Hence an alternative might be to > simply return an error if a vCPU doesn't have VPF_down set. With Remus/COLO, we should hit this path with an up vCPU on every iteration after the first. Given this bug, I'm struggling to see how it works at all. ~Andrew > > --- a/xen/arch/x86/hvm/save.c > +++ b/xen/arch/x86/hvm/save.c > @@ -268,7 +268,7 @@ int hvm_load(struct domain *d, hvm_domai > > /* Down all the vcpus: we only re-enable the ones that had state saved. > */ > for_each_vcpu(d, v) > -if ( test_and_set_bit(_VPF_down, &v->pause_flags) ) > +if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) > vcpu_sleep_nosync(v); > > for ( ; ; ) > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/HVM: correct an inverted check in hvm_load()
Clearly we want to put a vCPU to sleep if it is _not_ already down. Signed-off-by: Jan Beulich --- TBD: Since the flaw apparently never mattered, I imply that the function is never called with any vCPU up. Hence an alternative might be to simply return an error if a vCPU doesn't have VPF_down set. --- a/xen/arch/x86/hvm/save.c +++ b/xen/arch/x86/hvm/save.c @@ -268,7 +268,7 @@ int hvm_load(struct domain *d, hvm_domai /* Down all the vcpus: we only re-enable the ones that had state saved. */ for_each_vcpu(d, v) -if ( test_and_set_bit(_VPF_down, &v->pause_flags) ) +if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) vcpu_sleep_nosync(v); for ( ; ; ) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
