Re: [Xen-devel] [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 16 August 2018 08:34
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation
> does not cross GFN boundaries
> 
> >>> On 10.08.18 at 16:48, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -184,6 +184,25 @@ static int hvmemul_do_io(
> >          hvmtrace_io_assist(&p);
> >      }
> >
> > +    /*
> > +     * Make sure that we truncate rep MMIO at any GFN boundary. This is
> > +     * necessary to ensure that the correct device model is targetted
> > +     * or that we correctly handle a rep op spanning MMIO and RAM.
> > +     */
> > +    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
> > +    {
> > +        unsigned long off = p.addr & ~PAGE_MASK;
> > +
> > +        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
> > +            p.count = 1;
> > +        else
> > +            p.count = min_t(unsigned long,
> > +                            p.count,
> > +                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /
> > +                             p.size));
> > +    }
> > +    ASSERT(p.count);
> 
> I've applied the patch with the earlier suggested changes, but I wonder
> if the placement especially wrt the visible in context hvmtrace_io_assist()
> isn't sub-optimal.
> 
> Considering the other splitting done elsewhere I also wonder whether
> some of that can't then go away, or whether the specific code path
> where you've found the splitting to be missing wouldn't better be fixed
> instead. It certainly feels wrong for splitting to happen in multiple
> places for (I think) a single request.

Agreed. The issue appears to be that the higher layers in emulation are coded 
to split across a page boundary for the 'definitely memory' operand of a rep 
mov but not the mmio operand.

  Paul

> 
> I'll defer the decision whether to backport this until we've settled both
> of the above.
> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel