Re: [Xen-devel] hvmemul_virtual_to_linear() doesn't care about direction-flag?

[Keir Fraser <keir.fraser@xxxxxxxxxxxxx>]

On 19/8/08 18:04, "Byrne, John (HP Labs)" <john.l.byrne@xxxxxx> wrote:

Thanks for taking a look!

> Major:
> 
> 1.) In hvmemul_virtual_to_linear(), you've added the min() (line 299) on reps
> for reasons I don't understand and the ASSERT (line 304) in the reverse case.
> I don't see anything, anywhere, that guarantee that the ASSERT is true...and
> it needs to be for the code to be correct. If the min() is meant to guarantee
> this somehow, I don't see how it does. If it isn't meant to do this, I don't
> understand what it is for, as written.

The min() is to avoid overflow when multiplying by bytes_per_rep. It's
obviously a very conservative clip, but it's the same maximum we use in
hvmemul_linear_to_phys() so there's no point using a larger value. Changeset
18342 now adds a comment to this effect.

The assertion is subtle. Notice that on entry to the for-loop when
reverse=true then it is always the case that done >= bytes_per_rep. Hence
done/bytes_per_rep != 0 and the assertion must hold.

> Minor:
> 
> 2.) In hvmemul_linear_to_phys(), you changed the exception injection (line
> 265) to use (addr & PAGE_MASK) instead of addr? Seems wrong, but there could
> easily be something I don't understand.

In the forward (EFLAGS.DF=0) case, if we span multiple pages then a
page-fault on any other than the first page in the range will always have
the faulting linear address at the first byte of the page. The first page is
of course a special case but that's dealt with separately on line 240. I
changed to addr&PAGE_MASK because I changed how addr is updated in the loop.
Notice I no longer add done on the first iteration but now always add
PAGE_SIZE. Previously it was guaranteed that addr would be page-aligned --
now I have to force it when injecting the exception.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel