[PATCH 0/2] x86/hvm: ioreq server fixes

[PATCH 0/2] x86/hvm: ioreq server fixes

[Paul Durrant]

The following two patches fix a bug in the ioreq server gmfn release
code and also make the semantics of server destruction more intuitive.

[PATCH 1/2] x86/hvm: actually release ioreq server pages

[Paul Durrant]

hvm_free_ioreq_gmfn has the sense of the ioreq_gmfn mask inverted; it
needs to set a bit to release the gmfn, not clear it.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bfde380..f840175 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -496,7 +496,7 @@ static void hvm_free_ioreq_gmfn(struct domain *d, unsigned long gmfn)
 {
     unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
 
-    clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
+    set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
 }
 
 static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool_t buf)
-- 
1.7.10.4

Re: [PATCH 1/2] x86/hvm: actually release ioreq server pages

[Andrew Cooper]

On 23/04/15 16:46, Paul Durrant wrote:
> hvm_free_ioreq_gmfn has the sense of the ioreq_gmfn mask inverted; it
> needs to set a bit to release the gmfn, not clear it.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

s/mask/free_mask/ ?

For the change itself, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/hvm/hvm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index bfde380..f840175 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -496,7 +496,7 @@ static void hvm_free_ioreq_gmfn(struct domain *d, unsigned long gmfn)
>  {
>      unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
>  
> -    clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> +    set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>  }
>  
>  static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool_t buf)

Re: [PATCH 1/2] x86/hvm: actually release ioreq server pages

[Jan Beulich]

>>> On 23.04.15 at 17:46, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -496,7 +496,7 @@ static void hvm_free_ioreq_gmfn(struct domain *d, unsigned long gmfn)
>  {
>      unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
>  
> -    clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> +    set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>  }

Double checking this I came to wonder whether
HVM_PARAM_NR_IOREQ_SERVER_PAGES handling isn't broken
when invoked a second time with a.value smaller than what was
used the first time, or when any server pages are already in
use.

Jan

Re: [PATCH 1/2] x86/hvm: actually release ioreq server pages

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 24 April 2015 09:01
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: Re: [PATCH 1/2] x86/hvm: actually release ioreq server pages
> 
> >>> On 23.04.15 at 17:46, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -496,7 +496,7 @@ static void hvm_free_ioreq_gmfn(struct domain *d,
> unsigned long gmfn)
> >  {
> >      unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
> >
> > -    clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> > +    set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> >  }
> 
> Double checking this I came to wonder whether
> HVM_PARAM_NR_IOREQ_SERVER_PAGES handling isn't broken
> when invoked a second time with a.value smaller than what was
> used the first time, or when any server pages are already in
> use.
> 

Yes, the param is only intended to be a set-once param, the same as HVM_PARAM_IOREQ_SERVER_PFN. If the latter were changed after ioreq servers had been created then it could also lead to strangeness. I'll put together a patch to add checks for both.

  Paul

> Jan

[PATCH 2/2] x86/hvm: implicitly disable an ioreq server when it is destroyed

[Paul Durrant]

Currently, unless a (non-default) ioreq server is explicitly disabled before
being destroyed, its gmfns will not be placed back into the p2m but still
released back into the ioreq_gmfn mask. This is somewhat counter-intuitive
and easily remedied by this small patch.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f840175..3c62b80 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -972,6 +972,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, struct domain *d,
 static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s,
                                     bool_t is_default)
 {
+    ASSERT(!s->enabled);
     hvm_ioreq_server_remove_all_vcpus(s);
     hvm_ioreq_server_unmap_pages(s, is_default);
     hvm_ioreq_server_free_rangesets(s, is_default);
@@ -1074,6 +1075,8 @@ static int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
 
         domain_pause(d);
 
+        hvm_ioreq_server_disable(s, 0);
+
         list_del(&s->list_entry);
         
         hvm_ioreq_server_deinit(s, 0);
@@ -1332,11 +1335,10 @@ static void hvm_destroy_all_ioreq_servers(struct domain *d)
     {
         bool_t is_default = (s == d->arch.hvm_domain.default_ioreq_server);
 
+        hvm_ioreq_server_disable(s, is_default);
+
         if ( is_default )
-        {
-            hvm_ioreq_server_disable(s, 1);
             d->arch.hvm_domain.default_ioreq_server = NULL;
-        }
 
         list_del(&s->list_entry);
         
-- 
1.7.10.4

Re: [PATCH 2/2] x86/hvm: implicitly disable an ioreq server when it is destroyed

[Andrew Cooper]

On 23/04/15 16:46, Paul Durrant wrote:
> Currently, unless a (non-default) ioreq server is explicitly disabled before
> being destroyed, its gmfns will not be placed back into the p2m but still
> released back into the ioreq_gmfn mask. This is somewhat counter-intuitive
> and easily remedied by this small patch.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/hvm/hvm.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f840175..3c62b80 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -972,6 +972,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, struct domain *d,
>  static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s,
>                                      bool_t is_default)
>  {
> +    ASSERT(!s->enabled);
>      hvm_ioreq_server_remove_all_vcpus(s);
>      hvm_ioreq_server_unmap_pages(s, is_default);
>      hvm_ioreq_server_free_rangesets(s, is_default);
> @@ -1074,6 +1075,8 @@ static int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>  
>          domain_pause(d);
>  
> +        hvm_ioreq_server_disable(s, 0);
> +
>          list_del(&s->list_entry);
>          
>          hvm_ioreq_server_deinit(s, 0);
> @@ -1332,11 +1335,10 @@ static void hvm_destroy_all_ioreq_servers(struct domain *d)
>      {
>          bool_t is_default = (s == d->arch.hvm_domain.default_ioreq_server);
>  
> +        hvm_ioreq_server_disable(s, is_default);
> +
>          if ( is_default )
> -        {
> -            hvm_ioreq_server_disable(s, 1);
>              d->arch.hvm_domain.default_ioreq_server = NULL;
> -        }
>  
>          list_del(&s->list_entry);
>