Re: [Xen-devel] [PATCH v3 07/18] x86/hvm: unify internal portio and mmio intercepts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.15 at 12:39, <paul.durrant@xxxxxxxxxx> wrote:
> The implementation of mmio and portio intercepts is unnecessarily different.
> This leads to much code duplication. This patch unifies much of the
> intercept handling, leaving only distinct handlers for stdvga mmio and dpci
> portio. Subsequent patches will unify those handlers.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---

Please help reviewers of previous versions of your patches by
summarizing changes in the current version here.

> +static int hvm_portio_read(struct hvm_io_handler *handler,
> +                           uint64_t addr,
> +                           uint64_t size,
> +                           uint64_t *data)
>  {
> -    struct vcpu *curr = current;
> -    unsigned int i;
> +    uint32_t val = 0;
> +    int rc;
>  
> -    for ( i = 0; i < HVM_MMIO_HANDLER_NR; ++i )
> -        if ( hvm_mmio_handlers[i]->check(curr, gpa) )
> -            return 1;
> +    BUG_ON(handler->type != IOREQ_TYPE_PIO);
>  
> -    return 0;
> +    rc = handler->u.portio.action(IOREQ_READ, addr, size, &val);
> +    if ( rc == X86EMUL_OKAY )
> +        *data = val;

I think there would be no harm doing this unconditionally, and it
would eliminate the potential of the caller using uninitialized data.

> @@ -284,29 +185,37 @@ static int process_portio_intercept(portio_action_t 
> action, ioreq_t *p)
>      {
>          for ( i = 0; i < p->count; i++ )
>          {
> -            data = 0;
> -            switch ( hvm_copy_from_guest_phys(&data, p->data + step * i,
> -                                              p->size) )
> +            if ( p->data_is_ptr )
>              {
> -            case HVMCOPY_okay:
> -                break;
> -            case HVMCOPY_gfn_paged_out:
> -            case HVMCOPY_gfn_shared:
> -                rc = X86EMUL_RETRY;
> -                break;
> -            case HVMCOPY_bad_gfn_to_mfn:
> -                data = ~0;
> -                break;
> -            case HVMCOPY_bad_gva_to_gfn:
> -                ASSERT(0);
> -                /* fall through */
> -            default:
> -                rc = X86EMUL_UNHANDLEABLE;
> -                break;
> +                switch ( hvm_copy_from_guest_phys(&data, p->data + step * i,
> +                                                  p->size) )
> +                {
> +                case HVMCOPY_okay:
> +                    break;
> +                case HVMCOPY_gfn_paged_out:
> +                case HVMCOPY_gfn_shared:
> +                    rc = X86EMUL_RETRY;
> +                    break;
> +                case HVMCOPY_bad_gfn_to_mfn:
> +                    data = ~0;
> +                    break;
> +                case HVMCOPY_bad_gva_to_gfn:
> +                    ASSERT_UNREACHABLE();
> +                    /* fall through */
> +                default:
> +                    rc = X86EMUL_UNHANDLEABLE;
> +                    break;
> +                }
> +                if ( rc != X86EMUL_OKAY )
> +                    break;
>              }
> -            if ( rc != X86EMUL_OKAY )
> -                break;
> -            rc = action(IOREQ_WRITE, p->addr, p->size, &data);
> +            else
> +                data = p->data;
> +
> +            addr = (p->type == IOREQ_TYPE_COPY) ?
> +                p->addr + step * i :
> +                p->addr;

Indentation.

> @@ -324,78 +233,133 @@ static int process_portio_intercept(portio_action_t 
> action, ioreq_t *p)
>      return rc;
>  }
>  
> -/*
> - * Check if the request is handled inside xen
> - * return value: 0 --not handled; 1 --handled
> - */
> -int hvm_io_intercept(ioreq_t *p, int type)
> +static struct hvm_io_handler *hvm_find_io_handler(ioreq_t *p)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *curr_d = curr->domain;

"curr" is used only once (here) and hence pointless as a local variable.

> +    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];
> +        uint64_t start, end, count = p->count, size = p->size;

I'm not really happy with all these 64-bit local variables, but I
guess they are the result of you not wanting to do things the
way they're currently done everywhere... From a logical pov,
start and end would better be paddr_t, count and size
unsigned long.

> +        if ( handler->type != p->type )
> +            continue;
> +
> +        switch ( handler->type )
> +        {
> +        case IOREQ_TYPE_PIO:
> +            start = p->addr;
> +            end = p->addr + size;
> +            break;
> +        case IOREQ_TYPE_COPY:
> +            if ( p->df )
> +            {
> +                start = (p->addr - (count - 1) * size);
> +                end = p->addr + size;
> +            }
> +            else
> +            {
> +                start = p->addr;
> +                end = p->addr + count * size;
> +            }
> +            break;
> +        default:
> +            BUG();
> +        }

Doing this over and over again during each iteration looks kind of
wasteful.

> +static struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
>  {
> -    struct hvm_io_handler *handler = d->arch.hvm_domain.io_handler;
> -    int num = handler->num_slot;
> +    unsigned int i = d->arch.hvm_domain.io_handler_count++;
>  
> -    BUG_ON(num >= MAX_IO_HANDLER);
> +    if (i == NR_IO_HANDLERS)

Codingy style.

> +        domain_crash(d);

return NULL;

(or else you, despite having checked, return a pointer to an out of
bounds slot to your caller)

> +void relocate_portio_handler(struct domain *d, uint32_t old_addr,
> +                             uint32_t new_addr, uint32_t size)
> +{
> +    ioreq_t p = {
> +        .type = IOREQ_TYPE_PIO,
> +        .addr = old_addr,
> +        .size = size
> +    };
> +    struct hvm_io_handler *handler = hvm_find_io_handler(&p);

This isn't quite the same as the original checking - there was an
exact match of size required.

> +struct hvm_io_handler {
> +    union {
> +        struct {
> +            const struct hvm_mmio_ops *ops;
> +        } mmio;
> +        struct {
> +            uint32_t        start, end;
> +            portio_action_t action;
> +        } portio;
> +    } u;

No need to name this union, this is not a public interface.

Jan

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