You're welcome.
I was talking about a different ASSERT, see below, but I think I've answered my
own concern. Given that, I don't see any problems.
Thanks.
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@xxxxxxxxxxxxx]
> Sent: Tuesday, August 19, 2008 11:00 AM
> To: Byrne, John (HP Labs); xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] hvmemul_virtual_to_linear() doesn't care about
> direction-flag?
>
> 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.
Thanks.
>
> 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.
Wrong assert. I confused you, my fault. The ASSERT(!reverse) I figured out.
The ASSERT I was concerned about is this:
if ( (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1) )
{
ASSERT(offset >= ((*reps - 1) * bytes_per_rep));
However, I just read truncate_ea_and_reps() in the emulator which should
guarantee this.
> > 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.
Clear now, thanks. I wasn't reading it properly for some reason.
>
> -- Keir
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home