Re: [PATCH v3 04/18] x86/hvm: make sure translated MMIO reads or writes fall within a page

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 23 June 2015 17:22
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: RE: [PATCH v3 04/18] x86/hvm: make sure translated MMIO reads or
> writes fall within a page
> 
> >>> On 23.06.15 at 18:13, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 23 June 2015 16:53
> >> To: Paul Durrant
> >> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org)
> >> Subject: Re: [PATCH v3 04/18] x86/hvm: make sure translated MMIO
> reads or
> >> writes fall within a page
> >>
> >> >>> On 23.06.15 at 12:39, <paul.durrant@citrix.com> wrote:
> >> > ...otherwise they will simply carry on to the next page using a normal
> >> > linear-to-phys translation.
> >>
> >> And what's wrong about this?
> >
> > Is it the right thing to do? It seems wrong.
> 
> But why? Which way we obtain a linear-to-phys translation doesn't
> matter.
> 
> >> > --- a/xen/arch/x86/hvm/emulate.c
> >> > +++ b/xen/arch/x86/hvm/emulate.c
> >> > @@ -586,7 +586,6 @@ static int __hvmemul_read(
> >> >                                          p_data);
> >> >              if ( rc != X86EMUL_OKAY || bytes == chunk )
> >> >                  return rc;
> >> > -            addr += chunk;
> >> >              off += chunk;
> >> >              gpa += chunk;
> >> >              p_data += chunk;
> >> > @@ -594,6 +593,8 @@ static int __hvmemul_read(
> >> >              if ( bytes < chunk )
> >> >                  chunk = bytes;
> >> >          }
> >> > +
> >> > +        return X86EMUL_UNHANDLEABLE;
> >> >      }
> >>
> >> All the loop above does is leverage that we don't need to do a
> >> translation, as we already know the physical address. Continuing
> >> if the access crosses a page boundary is not wrong at all afaict.
> >>
> >
> > In what circumstances would you expect this to happen. My cscope
> showed up
> > handle_mmio_with_translation() being called by the shadow code and
> nested
> > page fault code.
> > The nested code does not look like it expects the I/O to cross a page
> > boundary. Particularly it appears to do checks on the gpa without
> considering
> > the possibility that the I/O may spill on to a subsequent page. Similarly the
> > shadow code appears to do a va to gpa lookup assuming again that the
> > translation is valid for the whole I/O.
> 
> Taking specific examples of current callers is not ideal. The code
> should cope with any (valid) caller, i.e. with any memory operand
> a valid instruction can have. And indeed I'd expect the calls from
> hvm_hap_nested_page_fault() to handle_mmio() to also have
> the potential to reach here (for arbitrary instructions accessing
> MMIO memory).
> 
> > I'm willing to admit that I only have a very basic knowledge of the code in
> > this area but, on the face of it, just walking onto the next linear address
> > after translation does not seem like it's the right thing to do.
> 
> That's how instructions work - they simply do another page
> translation if a memory access crosses a page boundary.
> 

Ok. If you believe it's the right thing to do, I'm happy to drop this patch out of the series. I'll send v4 tomorrow.

  Paul

> Jan
>