Re: [PATCH v5 06/16] x86/hvm: add length to mmio check op

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/06/15 14:05, Paul Durrant wrote:
> When memory mapped I/O is range checked by internal handlers, the length
> of the access should be taken into account.
>
> 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/intercept.c |   22 +++++++++++++++++++---
>  xen/include/asm-x86/hvm/io.h |   16 ++++++++++++++++
>  2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
> index 7d36785..42050f4 100644
> --- a/xen/arch/x86/hvm/intercept.c
> +++ b/xen/arch/x86/hvm/intercept.c
> @@ -35,9 +35,19 @@
>  static bool_t hvm_mmio_accept(const struct hvm_io_handler *handler,
>                                const ioreq_t *p)
>  {
> +    paddr_t first = hvm_mmio_first_byte(p);
> +    paddr_t last = hvm_mmio_last_byte(p);
> +
>      BUG_ON(handler->type != IOREQ_TYPE_COPY);
>  
> -    return handler->mmio.ops->check(current, p->addr);
> +    if ( !handler->mmio.ops->check(current, first) )
> +        return 0;
> +

I would put a comment here about an IO access straddling an MMIO handler
boundary, so that someone investigating this domain crash gets some clue
as to why.

> +    if ( p->size > 1 &&
> +         !handler->mmio.ops->check(current, last) )
> +        domain_crash(current->domain);
> +
> +    return 1;
>  }
>  
>  static int hvm_mmio_read(const struct hvm_io_handler *handler,
> @@ -112,7 +122,8 @@ static const struct hvm_io_ops portio_ops = {
>  static int hvm_process_io_intercept(const struct hvm_io_handler *handler,
>                                      ioreq_t *p)
>  {
> -    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> +    struct vcpu *curr = current;
> +    struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
>      const struct hvm_io_ops *ops =
>          (p->type == IOREQ_TYPE_COPY) ?
>          &mmio_ops :
> @@ -223,6 +234,9 @@ static int hvm_process_io_intercept(const struct hvm_io_handler *handler,
>  
>      if ( i != 0 )
>      {
> +        if ( rc == X86EMUL_UNHANDLEABLE )
> +            domain_crash(curr->domain);
> +
>          p->count = i;
>          rc = X86EMUL_OKAY;
>      }
> @@ -342,7 +356,9 @@ bool_t hvm_mmio_internal(paddr_t gpa)
>  {
>      ioreq_t p = {
>          .type = IOREQ_TYPE_COPY,
> -        .addr = gpa
> +        .addr = gpa,

As a general note, many compilers (gcc includes) permit having a comma
as the final token before the } which avoids a diff which looks like
this when adding a subsequent member.

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