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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 05/11/13 08:42, Jan Beulich wrote:
> > > > 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?
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

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