Re: [Xen-devel] [PATCH v14 12/17] pvh: Use PV handlers for cpuid, and IO

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.11.13 at 13:15, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> @@ -140,6 +146,9 @@ static int hvmemul_do_io(
>          }
>      }
>  
> +    if ( is_pvh_vcpu(curr) )
> +        ASSERT(vio->io_state == HVMIO_none);

Can we really get here for PVH?

> +static int pvhemul_do_pio(
> +    unsigned long port, int size, paddr_t ram_gpa, int dir, void *p_data)
> +{
> +    paddr_t value = ram_gpa;
> +    struct vcpu *curr = current;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +    /*
> +     * Weird-sized accesses have undefined behaviour: we discard writes
> +     * and read all-ones.
> +     */
> +    if ( unlikely((size > sizeof(long)) || (size & (size - 1))) )

I think you can safely ASSERT() here - PIO instructions never have
operand sizes not matching the criteria above.

> +    {
> +        gdprintk(XENLOG_WARNING, "bad mmio size %d\n", size);
> +        ASSERT(p_data != NULL); /* cannot happen with a REP prefix */
> +        if ( dir == IOREQ_READ )
> +            memset(p_data, ~0, size);
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    if ( dir == IOREQ_WRITE ) {
> +        if ( (p_data != NULL) )

Coding style (two instances).

> +        {
> +            memcpy(&value, p_data, size);
> +            p_data = NULL;
> +        }
> +
> +    if ( dir == IOREQ_WRITE )
> +        trace_io_assist(0, dir, 1, port, value);

Indentation (or really pointless if()).

> +
> +        guest_io_write(port, size, value, curr, regs);
> +    }
> +    else 
> +    {
> +        value = guest_io_read(port, size, curr, regs);
> +        trace_io_assist(0, dir, 1, port, value);
> +        if ( (p_data != NULL) )

Coding style again (sort of at least).

> +            memcpy(p_data, &value, size);
> +        memcpy(&regs->eax, &value, size);

What is this being matched by in (a) the HVM equivalent and (b)
the write code path? And even if needed, this surely wouldn't
be correct for the size == 4 case (where the upper 32 bits of
any destination register get zeroed).

Hmm, now that I take a second look, I see that this apparently
originates from handle_pio() (which however does the reading
of ->eax as well), so the above comment actually points out a
bug there (which I'm going to prepare a patch for right away).

> +    }
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +
>  int hvmemul_do_pio(
>      unsigned long port, unsigned long *reps, int size,
>      paddr_t ram_gpa, int dir, int df, void *p_data)
>  {
> -    return hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data);
> +    return is_hvm_vcpu(current) ?
> +        hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data) :
> +        pvhemul_do_pio(port, size, ram_gpa, dir, p_data);

You're losing "reps" and "df" here.

Jan


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