Re: [PATCH v4 08/17] x86/hvm: unify dpci portio intercept with standard portio intercept

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.06.15 at 13:24, <paul.durrant@citrix.com> wrote:
> @@ -237,16 +234,13 @@ static struct hvm_io_handler *hvm_find_io_handler(ioreq_t *p)
>  {
>      struct vcpu *curr = current;
>      struct domain *curr_d = curr->domain;
> -    const struct hvm_io_ops *ops =
> -        (p->type == IOREQ_TYPE_COPY) ?
> -        &mmio_ops :
> -        &portio_ops;
>      unsigned int i;
>  
>      for ( i = 0; i < curr_d->arch.hvm_domain.io_handler_count; i++ )
>      {
>          struct hvm_io_handler *handler =
>              &curr_d->arch.hvm_domain.io_handler[i];
> +        const struct hvm_io_ops *ops = handler->ops;

Which highlights that handler should probably also become const
(right where being introduced).

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -208,185 +208,100 @@ void hvm_io_assist(ioreq_t *p)
>      }
>  }
>  
> -static int dpci_ioport_read(uint32_t mport, ioreq_t *p)
> +static bool_t dpci_portio_accept(struct hvm_io_handler *handler,
> +                                 uint64_t addr,
> +                                 uint64_t size)
>  {
> -    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> -    int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size;
> -    uint32_t data = 0;
> +    struct vcpu *curr = current;
> +    struct hvm_iommu *hd = domain_hvm_iommu(curr->domain);
> +    struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> +    struct g2m_ioport *g2m_ioport;
> +    uint32_t start, end;
> +    uint32_t gport = addr, mport;

All four of them should be unsigned int. And the same would again
apply to earlier patches. There's no need for port numbers to be
fixed width other than uint16_t (which would be inefficient for
other reasons).

> + found:
> +    mport = (gport - start) + g2m_ioport->mport;
>  
> -    return rc;
> +    vio->g2m_ioport = g2m_ioport;
> +    return 1;
>  }

The value calculated into mport is not used.

> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -48,6 +48,8 @@ struct hvm_mmio_ops {
>  typedef int (*portio_action_t)(
>      int dir, uint32_t port, uint32_t bytes, uint32_t *val);
>  
> +struct hvm_io_ops;

This is not needed in an internal header when the use is ...

> @@ -58,6 +60,7 @@ struct hvm_io_handler {
>              portio_action_t action;
>          } portio;
>      } u;
> +    const struct hvm_io_ops *ops;

... in a structure, union, return value of a function, or declaration
of a global variable. Only function parameter types require such.

> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -77,6 +77,8 @@ struct hvm_vcpu_io {
>      bool_t mmio_retry, mmio_retrying;
>  
>      unsigned long msix_unmask_address;
> +
> +    struct g2m_ioport *g2m_ioport;
>  };

I think this should be const, documenting that users of the field
won't modify what it points to.

Jan