Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
> -Original Message- > From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of > Vitaly Kuznetsov > Sent: 05 September 2017 17:49 > To: Paul Durrant > Cc: Stefano Stabellini ; Wei Liu > ; Andrew Cooper ; Tim > (Xen.org) ; George Dunlap ; > xen-devel@lists.xen.org; Jan Beulich ; Ian Jackson > > Subject: Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft > reset > > Paul Durrant writes: > > >> Paul Durrant writes: > >> > >> > > >> > 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. > > > > And in addition to that we don't need the concept of > 'default_ioreq_server' and special pathes for it all over the code. That > would be ideal, but: > > I tried switching qemu-traditional to the new API and even succeeded, > everything including pci pass-through seems to work. However, I'm not > anywhere close to '20-30 lines' -- it's an order of magnitude more :-) > Well, the compat code does rather bloat it but I guess it is probably necessary. Xen has had ioreq servers for a long time but probably not long enough. > Anyway, here is the patch (attached). If everyone agrees the change is > appropriate for qemu-traditional I can sent it out. No additional > changes to the hypervisor is required. > LGTM so I think it's worth sending out. Cheers, Paul > -- > Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
Paul Durrant writes: >> Paul Durrant writes: >> >> > >> > 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. > And in addition to that we don't need the concept of 'default_ioreq_server' and special pathes for it all over the code. That would be ideal, but: I tried switching qemu-traditional to the new API and even succeeded, everything including pci pass-through seems to work. However, I'm not anywhere close to '20-30 lines' -- it's an order of magnitude more :-) Anyway, here is the patch (attached). If everyone agrees the change is appropriate for qemu-traditional I can sent it out. No additional changes to the hypervisor is required. -- Vitaly >From 030c73f4f0361752dad57a2a90179876ad697bfd Mon Sep 17 00:00:00 2001 From: Vitaly Kuznetsov Date: Tue, 5 Sep 2017 18:16:03 +0200 Subject: [PATCH] switch to the new IOREQ server API Signed-off-by: Vitaly Kuznetsov --- hw/pci.c| 5 ++ hw/xen_common.h | 169 hw/xen_machine_fv.c | 31 -- i386-dm/exec-dm.c | 7 +++ i386-dm/helper2.c | 31 +++--- vl.c| 9 +++ xen-vl-extra.c | 3 + 7 files changed, 242 insertions(+), 13 deletions(-) diff --git a/hw/pci.c b/hw/pci.c index c4232856..d6cafb3e 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -34,6 +34,7 @@ #ifdef CONFIG_PASSTHROUGH #include "hw/pass-through.h" #endif +#include "hw/xen_common.h" extern int igd_passthru; @@ -248,6 +249,10 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name, return NULL; found: ; } + +xen_map_pcidev(xc_handle, domid, ioservid, 0, + PCI_SLOT(devfn), PCI_FUNC(devfn)); + pci_dev = qemu_mallocz(instance_size); pci_dev->bus = bus; pci_dev->devfn = devfn; diff --git a/hw/xen_common.h b/hw/xen_common.h index cc48892f..cde814dc 100644 --- a/hw/xen_common.h +++ b/hw/xen_common.h @@ -33,4 +33,173 @@ # define xen_wmb() wmb() #endif +extern uint16_t ioservid; + +#if __XEN_LATEST_INTERFACE_VERSION__ < 0x00040500 +static inline int xen_create_ioreq_server(xc_interface *xc, domid_t dom, + uint16_t *ioservid) +{ +return 0; +} + +static inline int xen_get_ioreq_server_info(xc_interface *xc, domid_t dom, +uint16_t ioservid, +xen_pfn_t *ioreq_pfn, +xen_pfn_t *bufioreq_pfn, +uint32_t *bufioreq_evtchn) +{ +unsigned long param; +int rc; + +rc = xc_get_hvm_param(xc, dom, HVM_PARAM_IOREQ_PFN, ¶m); +if (rc < 0) { +fprintf(stderr, "failed to get HVM_PARAM_IOREQ_PFN\n"); +return -1; +} + +*ioreq_pfn = param; + +rc = xc_get_hvm_param(xc, dom, HVM_PARAM_BUFIOREQ_PFN, ¶m); +if (rc < 0) { +fprintf(stderr, "failed to get HVM_PARAM_BUFIOREQ_PFN\n"); +return -1; +} + +*bufioreq_pfn = param; + +rc = xc_get_hvm_param(xc, dom, HVM_PARAM_BUFIOREQ_EVTCHN, + ¶m); +if (rc < 0) { +fprintf(stderr, "failed to get HVM_PARAM_BUFIOREQ_EVTCHN\n"); +return -1; +} + +*bufioreq_evtchn = param; + +return 0; +} + +static inline int xen_set_ioreq_server_state(xc_interface *xc, domid_t dom, + uint16_t ioservid, + bool enable) +{ +return 0; +} + +static inline void xen_map_memory_section(xc_interface *xc, domid_t dom, + uint16_t ioservid, + uint64_t start, uint64_t end) +{ +} + +static inline void xen_unmap_memory_section(xc_interface *xc, domid_t dom, +uint16_t ioservid, + uint64_t start, uint64_t end) +{ +} + +static inline void xen_map_io_section(xc_interface *xc, domid_t dom, + uint16_t ioservid, + uint64_t start, uint64_t end) +{ +} + +static inline void xen_unmap_io_section(xc_interface *xc, domid_t dom, +uint16_t ioservid, + uint64_t start, uint64_t end) +{ +} + +static inline void xen_map_pcidev(xc_interface *xc, domid_t dom, + uint16_t ioservid, + uint8_t bus, uint8_t device, + uint8_t function) +{ +} + +static inline void xen_unmap_pcidev(xc_interface *xc, domid_t dom, +uint16_t ioservid, +uint8_t bus, uint8_t device, +
Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: 01 September 2017 11:43 > To: Paul Durrant > Cc: Andrew Cooper ; xen- > de...@lists.xen.org; George Dunlap ; Ian > Jackson ; Jan Beulich ; > Konrad Rzeszutek Wilk ; Stefano Stabellini > ; Tim (Xen.org) ; Wei Liu > > Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset > > Paul Durrant writes: > > >> -Original Message- > >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > >> Sent: 01 September 2017 10:27 > >> To: Andrew Cooper ; Paul Durrant > >> > >> Cc: xen-devel@lists.xen.org; George Dunlap > ; > >> Ian Jackson ; Jan Beulich ; > >> Konrad Rzeszutek Wilk ; Stefano Stabellini > >> ; Tim (Xen.org) ; Wei Liu > >> > >> Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset > >> > >> Andrew Cooper 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 > >> > > >> > 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
Paul Durrant writes: >> -Original Message- >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] >> Sent: 01 September 2017 10:27 >> To: Andrew Cooper ; Paul Durrant >> >> Cc: xen-devel@lists.xen.org; George Dunlap ; >> Ian Jackson ; Jan Beulich ; >> Konrad Rzeszutek Wilk ; Stefano Stabellini >> ; Tim (Xen.org) ; Wei Liu >> >> Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset >> >> Andrew Cooper 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 >> > >> > 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! -- Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: 01 September 2017 10:27 > To: Andrew Cooper ; Paul Durrant > > Cc: xen-devel@lists.xen.org; George Dunlap ; > Ian Jackson ; Jan Beulich ; > Konrad Rzeszutek Wilk ; Stefano Stabellini > ; Tim (Xen.org) ; Wei Liu > > Subject: Re: [PATCH] xen: reset creation_finished flag on soft reset > > Andrew Cooper 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 > > > > 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
Andrew Cooper 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 > > 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, -- Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
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 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); ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset
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 --- 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); -- 2.13.5 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel