Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset

2017-09-06 Thread Paul Durrant 
> -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 <paul.durr...@citrix.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> <wei.l...@citrix.com>; Andrew Cooper <andrew.coop...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; George Dunlap <george.dun...@citrix.com>;
> xen-devel@lists.xen.org; Jan Beulich <jbeul...@suse.com>; Ian Jackson
> <ian.jack...@citrix.com>
> Subject: Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft
> reset
> 
> Paul Durrant <paul.durr...@citrix.com> writes:
> 
> >> Paul Durrant <paul.durr...@citrix.com> 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

2017-09-05 Thread Vitaly Kuznetsov 
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, );
+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, );
+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,
+  );
+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,
+

Re: [Xen-devel] [PATCH] xen: reset creation_finished flag on soft reset

2017-09-01 Thread Paul Durrant 
> -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

2017-09-01 Thread Vitaly Kuznetsov 
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

2017-09-01 Thread Paul Durrant 
> -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

2017-09-01 Thread Vitaly Kuznetsov 
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

2017-09-01 Thread Andrew Cooper 
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

2017-09-01 Thread Vitaly Kuznetsov 
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