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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 05/11/13 08:42, Jan Beulich wrote:
>>>> On 04.11.13 at 13:15, George Dunlap <george.dunlap@eu.citrix.com> 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?

Nope -- sorry I missed that one. :-)

>
>> +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()).

Oops...

>
>> +
>> +        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.

Hmm... yes.  Time to do some re-thinking on this one.

  -George