Re: [Xen-devel] [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Paul Durrant]

> -Original Message-
> From: Suravee Suthikulpanit [mailto:suravee.suthikulpa...@amd.com]
> Sent: 16 May 2016 17:03
> To: Paul Durrant; xen-devel@lists.xen.org; George Dunlap;
> jbeul...@suse.com
> Subject: Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> 
> Hi Paul,
> 
> On 05/16/2016 03:01 AM, Paul Durrant wrote:
> >> -Original Message-
> >> >From:suravee.suthikulpa...@amd.com
> >> >[mailto:suravee.suthikulpa...@amd.com]
> >> >Sent: 13 May 2016 20:37
> >> >To:xen-devel@lists.xen.org; George Dunlap;jbeul...@suse.com
> >> >Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> >> >Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> >> >
> >> >From: Suravee Suthikulpanit
> >> >
> >> >At the time of registering HVM I/O handler, the HVM domain might
> >> >not have been initialized,
> > I/O handler registration is internal to Xen so any caller that attempt to
> register before domain initialization should be removed and replaced with
> one that does it at the right time.
> 
> Ok. I'll just remove that call for now.
> 
> >> >which means the hvm_domain.io_handler
> >> >would be NULL. In the hvm_next_io_handler(), this should be checked
> >> >before returning and referencing the array. Also, the io_handler_count
> >> >should only be incremented on success.
> >> >
> >> >So, this patch adds error handling in hvm_next_io_handler.
> >> >
> > This isn't necessary. An ASSERT would be preferable so that buggy callers
> can be easily caught.
> 
> Ok, I'll update the patch to ASSERT() and send it out. Although, just
> want to make sure that you think it should really be doing assert and
> not warning + handling error? It seems quite aggressive to crash the
> hypervisor simply because some io handler are not properly call.
> 

That's a reasonable argument, but IMO since all the callers are in the 
hypervisor and ASSERTs only affect debug builds I think it's reasonable.

  Paul

> Thanks,
> Suravee

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Suravee Suthikulpanit]

Hi Paul,

On 05/16/2016 03:01 AM, Paul Durrant wrote:

-Original Message-
>From:suravee.suthikulpa...@amd.com
>[mailto:suravee.suthikulpa...@amd.com]
>Sent: 13 May 2016 20:37
>To:xen-devel@lists.xen.org; George Dunlap;jbeul...@suse.com
>Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
>Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
>
>From: Suravee Suthikulpanit
>
>At the time of registering HVM I/O handler, the HVM domain might
>not have been initialized,

I/O handler registration is internal to Xen so any caller that attempt to 
register before domain initialization should be removed and replaced with one 
that does it at the right time.


Ok. I'll just remove that call for now.


>which means the hvm_domain.io_handler
>would be NULL. In the hvm_next_io_handler(), this should be checked
>before returning and referencing the array. Also, the io_handler_count
>should only be incremented on success.
>
>So, this patch adds error handling in hvm_next_io_handler.
>

This isn't necessary. An ASSERT would be preferable so that buggy callers can 
be easily caught.


Ok, I'll update the patch to ASSERT() and send it out. Although, just 
want to make sure that you think it should really be doing assert and 
not warning + handling error? It seems quite aggressive to crash the 
hypervisor simply because some io handler are not properly call.


Thanks,
Suravee

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Paul Durrant]

> -Original Message-
> From: suravee.suthikulpa...@amd.com
> [mailto:suravee.suthikulpa...@amd.com]
> Sent: 13 May 2016 20:37
> To: xen-devel@lists.xen.org; George Dunlap; jbeul...@suse.com
> Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> 
> From: Suravee Suthikulpanit 
> 
> At the time of registering HVM I/O handler, the HVM domain might
> not have been initialized,

I/O handler registration is internal to Xen so any caller that attempt to 
register before domain initialization should be removed and replaced with one 
that does it at the right time.

> which means the hvm_domain.io_handler
> would be NULL. In the hvm_next_io_handler(), this should be checked
> before returning and referencing the array. Also, the io_handler_count
> should only be incremented on success.
> 
> So, this patch adds error handling in hvm_next_io_handler.
> 

This isn't necessary. An ASSERT would be preferable so that buggy callers can 
be easily caught.

  Paul

> Signed-off-by: Suravee Suthikulpanit 
> ---
>  xen/arch/x86/hvm/intercept.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
> index 7096d74..b837f5f 100644
> --- a/xen/arch/x86/hvm/intercept.c
> +++ b/xen/arch/x86/hvm/intercept.c
> @@ -248,7 +248,10 @@ int hvm_io_intercept(ioreq_t *p)
> 
>  struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
>  {
> -unsigned int i = d->arch.hvm_domain.io_handler_count++;
> +unsigned int i = d->arch.hvm_domain.io_handler_count;
> +
> +if ( !d->arch.hvm_domain.io_handler )
> +return NULL;
> 
>  if ( i == NR_IO_HANDLERS )
>  {
> @@ -256,6 +259,7 @@ struct hvm_io_handler *hvm_next_io_handler(struct
> domain *d)
>  return NULL;
>  }
> 
> +d->arch.hvm_domain.io_handler_count++;
>  return >arch.hvm_domain.io_handler[i];
>  }
> 
> --
> 1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2 1/2] x86/hvm: Add check when register io handler

[suravee.suthikulpanit]

From: Suravee Suthikulpanit 

At the time of registering HVM I/O handler, the HVM domain might
not have been initialized, which means the hvm_domain.io_handler
would be NULL. In the hvm_next_io_handler(), this should be checked
before returning and referencing the array. Also, the io_handler_count
should only be incremented on success.

So, this patch adds error handling in hvm_next_io_handler.

Signed-off-by: Suravee Suthikulpanit 
---
 xen/arch/x86/hvm/intercept.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
index 7096d74..b837f5f 100644
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -248,7 +248,10 @@ int hvm_io_intercept(ioreq_t *p)
 
 struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
 {
-unsigned int i = d->arch.hvm_domain.io_handler_count++;
+unsigned int i = d->arch.hvm_domain.io_handler_count;
+
+if ( !d->arch.hvm_domain.io_handler )
+return NULL;
 
 if ( i == NR_IO_HANDLERS )
 {
@@ -256,6 +259,7 @@ struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
 return NULL;
 }
 
+d->arch.hvm_domain.io_handler_count++;
 return >arch.hvm_domain.io_handler[i];
 }
 
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel