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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 August 2018 12:14
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [PATCH 2/2] x86/hvm/emulate: make sure rep I/O emulation
> does not cross GFN boundaries
> 
> On 10/08/18 11:37, Paul Durrant wrote:
> > When emulating a rep I/O operation it is possible that the ioreq will
> > describe a single operation that spans multiple GFNs. This is fine as long
> > as all those GFNs fall within an MMIO region covered by a single device
> > model, but unfortunately the higher levels of the emulation code do not
> > guarantee that. This is something that should almost certainly be fixed,
> > but in the meantime this patch makes sure that MMIO is truncated at GFN
> > boundaries and hence the appropriate device model is re-evaluated for
> each
> > target GFN.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/emulate.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index 8385c62145..d6a81ec4d1 100644
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -184,8 +184,23 @@ static int hvmemul_do_io(
> >          hvmtrace_io_assist(&p);
> >      }
> >
> > -    vio->io_req = 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;
> >
> > +        p.count = min_t(unsigned long,
> > +                        p.count,
> > +                        p.df ?
> > +                        (off + p.size) / p.size :
> > +                        (PAGE_SIZE - off) / p.size);
> 
> (p.df ? (off + p.size) : (PAGE_SIZE - off)) / p.size
> 

Yes, I suppose so.

> ?
> 
> > +    }
> > +
> > +    vio->io_req = p;
> 
> You'll get a cleaner patch if you retain the newline between these two.
> 
> Both can be fixed up on commit.  From a functional point of view, this
> looks fine.
> 

Ok. I'll leave to you then :-)

  Paul

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