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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.15 at 18:13, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 23 June 2015 16:53
>> To: Paul Durrant
>> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxxx> 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.

Jan



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