| Message ID | 20170901091128.21945-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/09/2017 10:11, Vitaly Kuznetsov wrote: > C/s e7dabe5 ("x86/hvm: don't unconditionally create a default ioreq > server") broke soft reset when QEMU traditional is being used. During > soft reset QEMU is relaunched and default ioreq server needs to be > re-created upon first HVM_PARAM_*IOREQ_* request. The flag will be > set back to 'true' when toolstack unpauses the domain, just like after > normal creation. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> Sorry, but nack. d->creation_finished is used for a number of things, one being TLB safety before the vcpus have started executing. We either need to split the variable, or rework e7dabe5 to not use this. ~Andrew > --- > xen/common/domain.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index b22aacc57e..b529c5d7ad 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1085,6 +1085,8 @@ int domain_soft_reset(struct domain *d) > unmap_vcpu_info(v); > } > > + d->creation_finished = false; > + > rc = arch_domain_soft_reset(d); > if ( !rc ) > domain_resume(d);
Andrew Cooper <andrew.cooper3@citrix.com> writes: > On 01/09/2017 10:11, Vitaly Kuznetsov wrote: >> C/s e7dabe5 ("x86/hvm: don't unconditionally create a default ioreq >> server") broke soft reset when QEMU traditional is being used. During >> soft reset QEMU is relaunched and default ioreq server needs to be >> re-created upon first HVM_PARAM_*IOREQ_* request. The flag will be >> set back to 'true' when toolstack unpauses the domain, just like after >> normal creation. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > Sorry, but nack. d->creation_finished is used for a number of things, > one being TLB safety before the vcpus have started executing. > > We either need to split the variable, or rework e7dabe5 to not use this. > I think that adding another flag is a bad idea, even 'creation_finished' flag looks a bit hackish to me. Adjusting e7dabe5 is probably better. However, while reading its blurb I don't fully understand the change: on migration we create new domain and thus reset creation_finished. During QEMU launch we still need to create ioreq server. Paul, could you please elaborate a bit (e.g. what are we guarding against, when creating ioreq server is redundant) so we can suggest a fix for soft reset? Thanks,
> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: 01 September 2017 10:27 > To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant > <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xen.org; George Dunlap <George.Dunlap@citrix.com>; > Ian Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini > <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu > <wei.liu2@citrix.com> > Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset > > Andrew Cooper <andrew.cooper3@citrix.com> writes: > > > On 01/09/2017 10:11, Vitaly Kuznetsov wrote: > >> C/s e7dabe5 ("x86/hvm: don't unconditionally create a default ioreq > >> server") broke soft reset when QEMU traditional is being used. During > >> soft reset QEMU is relaunched and default ioreq server needs to be > >> re-created upon first HVM_PARAM_*IOREQ_* request. The flag will be > >> set back to 'true' when toolstack unpauses the domain, just like after > >> normal creation. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > > Sorry, but nack. d->creation_finished is used for a number of things, > > one being TLB safety before the vcpus have started executing. > > > > We either need to split the variable, or rework e7dabe5 to not use this. > > > > I think that adding another flag is a bad idea, even 'creation_finished' > flag looks a bit hackish to me. Adjusting e7dabe5 is probably > better. However, while reading its blurb I don't fully understand the > change: on migration we create new domain and thus reset > creation_finished. During QEMU launch we still need to create ioreq > server. Paul, could you please elaborate a bit (e.g. what are we > guarding against, when creating ioreq server is redundant) so we can > suggest a fix for soft reset? My memory is hazy as to the exact problem, but I think it was an issue with the COLO project. IIRC they repeatedly 'migrate' a VM but then resume the original. Without e7dabe5 the sending VM ends up with a default ioreq server after the first migration because the save code reads the HVM params that trigger its creation. I wonder whether the easiest thing to do would be to modify qemu trad to do explicit ioreq server creation? It's really not that much code-change... 20-30 lines or so. Paul > > Thanks, > > -- > Vitaly
Paul Durrant <Paul.Durrant@citrix.com> writes: >> -----Original Message----- >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] >> Sent: 01 September 2017 10:27 >> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant >> <Paul.Durrant@citrix.com> >> Cc: xen-devel@lists.xen.org; George Dunlap <George.Dunlap@citrix.com>; >> Ian Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu >> <wei.liu2@citrix.com> >> Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset >> >> Andrew Cooper <andrew.cooper3@citrix.com> writes: >> >> > On 01/09/2017 10:11, Vitaly Kuznetsov wrote: >> >> C/s e7dabe5 ("x86/hvm: don't unconditionally create a default ioreq >> >> server") broke soft reset when QEMU traditional is being used. During >> >> soft reset QEMU is relaunched and default ioreq server needs to be >> >> re-created upon first HVM_PARAM_*IOREQ_* request. The flag will be >> >> set back to 'true' when toolstack unpauses the domain, just like after >> >> normal creation. >> >> >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> > >> > Sorry, but nack. d->creation_finished is used for a number of things, >> > one being TLB safety before the vcpus have started executing. >> > >> > We either need to split the variable, or rework e7dabe5 to not use this. >> > >> >> I think that adding another flag is a bad idea, even 'creation_finished' >> flag looks a bit hackish to me. Adjusting e7dabe5 is probably >> better. However, while reading its blurb I don't fully understand the >> change: on migration we create new domain and thus reset >> creation_finished. During QEMU launch we still need to create ioreq >> server. Paul, could you please elaborate a bit (e.g. what are we >> guarding against, when creating ioreq server is redundant) so we can >> suggest a fix for soft reset? > > My memory is hazy as to the exact problem, but I think it was an issue > with the COLO project. IIRC they repeatedly 'migrate' a VM but then > resume the original. Without e7dabe5 the sending VM ends up with a > default ioreq server after the first migration because the save code > reads the HVM params that trigger its creation. > > I wonder whether the easiest thing to do would be to modify qemu trad > to do explicit ioreq server creation? It's really not that much > code-change... 20-30 lines or so. I was thinking about this too, I'll try. It will hopefuly allow to get rid of the 'side effect' which creates default ioreq server on HVM parameters read. Thanks!
> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: 01 September 2017 11:43 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xen.org; George Dunlap <George.Dunlap@citrix.com>; Ian > Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini > <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu > <wei.liu2@citrix.com> > Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > >> -----Original Message----- > >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > >> Sent: 01 September 2017 10:27 > >> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant > >> <Paul.Durrant@citrix.com> > >> Cc: xen-devel@lists.xen.org; George Dunlap > <George.Dunlap@citrix.com>; > >> Ian Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; > >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini > >> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu > >> <wei.liu2@citrix.com> > >> Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset > >> > >> Andrew Cooper <andrew.cooper3@citrix.com> writes: > >> > >> > On 01/09/2017 10:11, Vitaly Kuznetsov wrote: > >> >> C/s e7dabe5 ("x86/hvm: don't unconditionally create a default ioreq > >> >> server") broke soft reset when QEMU traditional is being used. During > >> >> soft reset QEMU is relaunched and default ioreq server needs to be > >> >> re-created upon first HVM_PARAM_*IOREQ_* request. The flag will > be > >> >> set back to 'true' when toolstack unpauses the domain, just like after > >> >> normal creation. > >> >> > >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> > > >> > Sorry, but nack. d->creation_finished is used for a number of things, > >> > one being TLB safety before the vcpus have started executing. > >> > > >> > We either need to split the variable, or rework e7dabe5 to not use this. > >> > > >> > >> I think that adding another flag is a bad idea, even 'creation_finished' > >> flag looks a bit hackish to me. Adjusting e7dabe5 is probably > >> better. However, while reading its blurb I don't fully understand the > >> change: on migration we create new domain and thus reset > >> creation_finished. During QEMU launch we still need to create ioreq > >> server. Paul, could you please elaborate a bit (e.g. what are we > >> guarding against, when creating ioreq server is redundant) so we can > >> suggest a fix for soft reset? > > > > My memory is hazy as to the exact problem, but I think it was an issue > > with the COLO project. IIRC they repeatedly 'migrate' a VM but then > > resume the original. Without e7dabe5 the sending VM ends up with a > > default ioreq server after the first migration because the save code > > reads the HVM params that trigger its creation. > > > > I wonder whether the easiest thing to do would be to modify qemu trad > > to do explicit ioreq server creation? It's really not that much > > code-change... 20-30 lines or so. > > I was thinking about this too, I'll try. It will hopefuly allow to get > rid of the 'side effect' which creates default ioreq server on HVM > parameters read. Yes indeed. At that point I'd actually propose getting rid of those params altogether since nothing will use them anymore. Cheers, Paul > > Thanks! > > -- > Vitaly
diff --git a/xen/common/domain.c b/xen/common/domain.c index b22aacc57e..b529c5d7ad 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1085,6 +1085,8 @@ int domain_soft_reset(struct domain *d) unmap_vcpu_info(v); } + d->creation_finished = false; + rc = arch_domain_soft_reset(d); if ( !rc ) domain_resume(d);
C/s e7dabe5 ("x86/hvm: don't unconditionally create a default ioreq server") broke soft reset when QEMU traditional is being used. During soft reset QEMU is relaunched and default ioreq server needs to be re-created upon first HVM_PARAM_*IOREQ_* request. The flag will be set back to 'true' when toolstack unpauses the domain, just like after normal creation. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- xen/common/domain.c | 2 ++ 1 file changed, 2 insertions(+)