Byrne, John (HP Labs)
2008-Aug-18 18:41 UTC
[Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
I was following the emulation code around in xen-unstable cs 18335 and I noticed that the direction flag doesn''t get taken into account for the segment bounds checking in the 32-bit case anywhere I could see. Does anyone know better? Maybe no one will care, but I thought I''d mention it. John Byrne _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Aug-19 08:20 UTC
Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
Quite a nasty omission though, and quite easily fixed. Thanks for pointing it out. -- Keir On 18/8/08 19:41, "Byrne, John (HP Labs)" <john.l.byrne@hp.com> wrote:> I was following the emulation code around in xen-unstable cs 18335 and I > noticed that the direction flag doesn''t get taken into account for the segment > bounds checking in the 32-bit case anywhere I could see. Does anyone know > better? > > Maybe no one will care, but I thought I''d mention it. > > John Byrne > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Aug-19 10:48 UTC
Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
This affects hvmemul_linear_to_phys() too, and would for example mean that a backwards I/O string instruction in userspace that crosses a page boundary would very likely cause I/O to/from the wrong physical pages. I''ve confirmed this with a small testing patch to hvmloader. I think we need to work out how to maintain a test suite of this kind of thing to check for regressions in these kinds of rarer corner cases. Obviously I''ll fix this for 3.3.0 and probably roll out another release candidate. -- Keir On 19/8/08 09:20, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:> Quite a nasty omission though, and quite easily fixed. Thanks for pointing > it out. > > -- Keir > > On 18/8/08 19:41, "Byrne, John (HP Labs)" <john.l.byrne@hp.com> wrote: > >> I was following the emulation code around in xen-unstable cs 18335 and I >> noticed that the direction flag doesn''t get taken into account for the >> segment >> bounds checking in the 32-bit case anywhere I could see. Does anyone know >> better? >> >> Maybe no one will care, but I thought I''d mention it. >> >> John Byrne >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Aug-19 15:04 UTC
Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
This should be fixed as of c/s 18340. Feel free to see if you can spot any problems with it! I''ll roll another release candidate tomorrow morning, after the tree has been through automated testing. Curently it''s in staging only: http://xenbits.xensource.com/staging/xen-unstable.hg -- Keir On 19/8/08 11:48, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:> This affects hvmemul_linear_to_phys() too, and would for example mean that a > backwards I/O string instruction in userspace that crosses a page boundary > would very likely cause I/O to/from the wrong physical pages. I''ve confirmed > this with a small testing patch to hvmloader. I think we need to work out how > to maintain a test suite of this kind of thing to check for regressions in > these kinds of rarer corner cases. > > Obviously I''ll fix this for 3.3.0 and probably roll out another release > candidate. > > -- Keir > > On 19/8/08 09:20, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > >> Quite a nasty omission though, and quite easily fixed. Thanks for pointing >> it out. >> >> -- Keir >> >> On 18/8/08 19:41, "Byrne, John (HP Labs)" <john.l.byrne@hp.com> wrote: >> >>> I was following the emulation code around in xen-unstable cs 18335 and I >>> noticed that the direction flag doesn''t get taken into account for the >>> segment >>> bounds checking in the 32-bit case anywhere I could see. Does anyone know >>> better? >>> >>> Maybe no one will care, but I thought I''d mention it. >>> >>> John Byrne >>> >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xensource.com >>> http://lists.xensource.com/xen-devel >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Trolle Selander
2008-Aug-19 16:41 UTC
Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
Got a build error on CentOS 5.2/i386 (but not on Fedora 8 x86_64) emulate.c: In function ''hvmemul_linear_to_phys'': emulate.c:233: warning: passing argument 2 of ''hvmemul_linear_to_phys'' from incompatible pointer type Patch attached. -- Trolle On Tue, Aug 19, 2008 at 4:04 PM, Keir Fraser <keir.fraser@eu.citrix.com> wrote:> This should be fixed as of c/s 18340. Feel free to see if you can spot any > problems with it! I''ll roll another release candidate tomorrow morning, > after the tree has been through automated testing. > > Curently it''s in staging only: > http://xenbits.xensource.com/staging/xen-unstable.hg > > -- Keir > > On 19/8/08 11:48, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > >> This affects hvmemul_linear_to_phys() too, and would for example mean that a >> backwards I/O string instruction in userspace that crosses a page boundary >> would very likely cause I/O to/from the wrong physical pages. I''ve confirmed >> this with a small testing patch to hvmloader. I think we need to work out how >> to maintain a test suite of this kind of thing to check for regressions in >> these kinds of rarer corner cases. >> >> Obviously I''ll fix this for 3.3.0 and probably roll out another release >> candidate. >> >> -- Keir >> >> On 19/8/08 09:20, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: >> >>> Quite a nasty omission though, and quite easily fixed. Thanks for pointing >>> it out. >>> >>> -- Keir >>> >>> On 18/8/08 19:41, "Byrne, John (HP Labs)" <john.l.byrne@hp.com> wrote: >>> >>>> I was following the emulation code around in xen-unstable cs 18335 and I >>>> noticed that the direction flag doesn''t get taken into account for the >>>> segment >>>> bounds checking in the 32-bit case anywhere I could see. Does anyone know >>>> better? >>>> >>>> Maybe no one will care, but I thought I''d mention it. >>>> >>>> John Byrne >>>> >>>> >>>> _______________________________________________ >>>> Xen-devel mailing list >>>> Xen-devel@lists.xensource.com >>>> http://lists.xensource.com/xen-devel >>> > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Byrne, John (HP Labs)
2008-Aug-19 17:04 UTC
RE: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
Keir, "Quite easily fixed." You''ve been doing this too long. I''m glad you looked at it since you found all this and I likely wouldn''t have without being slapped upside the head to look deeper. I don''t have a test case, so I have been reading the code and testing the edge cases in my head and most of it looks good. Questions: 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. 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. John Byrne> -----Original Message----- > From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] > Sent: Tuesday, August 19, 2008 8:05 AM > To: Byrne, John (HP Labs); xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about > direction-flag? > > This should be fixed as of c/s 18340. Feel free to see if you can spot > any > problems with it! I''ll roll another release candidate tomorrow morning, > after the tree has been through automated testing. > > Curently it''s in staging only: > http://xenbits.xensource.com/staging/xen-unstable.hg > > -- Keir > > On 19/8/08 11:48, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > > > This affects hvmemul_linear_to_phys() too, and would for example mean > that a > > backwards I/O string instruction in userspace that crosses a page > boundary > > would very likely cause I/O to/from the wrong physical pages. I''ve > confirmed > > this with a small testing patch to hvmloader. I think we need to work > out how > > to maintain a test suite of this kind of thing to check for > regressions in > > these kinds of rarer corner cases. > > > > Obviously I''ll fix this for 3.3.0 and probably roll out another > release > > candidate. > > > > -- Keir > > > > On 19/8/08 09:20, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > > > >> Quite a nasty omission though, and quite easily fixed. Thanks for > pointing > >> it out. > >> > >> -- Keir > >> > >> On 18/8/08 19:41, "Byrne, John (HP Labs)" <john.l.byrne@hp.com> > wrote: > >> > >>> I was following the emulation code around in xen-unstable cs 18335 > and I > >>> noticed that the direction flag doesn''t get taken into account for > the > >>> segment > >>> bounds checking in the 32-bit case anywhere I could see. Does > anyone know > >>> better? > >>> > >>> Maybe no one will care, but I thought I''d mention it. > >>> > >>> John Byrne > >>> > >>> > >>> _______________________________________________ > >>> Xen-devel mailing list > >>> Xen-devel@lists.xensource.com > >>> http://lists.xensource.com/xen-devel > >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Aug-19 17:59 UTC
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@hp.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Byrne, John (HP Labs)
2008-Aug-19 18:33 UTC
RE: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
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@eu.citrix.com] > Sent: Tuesday, August 19, 2008 11:00 AM > To: Byrne, John (HP Labs); xen-devel@lists.xensource.com > 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@hp.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Aug-19 18:41 UTC
Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
On 19/8/08 19:33, "Byrne, John (HP Labs)" <john.l.byrne@hp.com> wrote:> 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.Yes, I considered giving that one a comment too. Now I will do so. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Trolle Selander
2008-Aug-20 11:35 UTC
Re: [Xen-devel] hvmemul_virtual_to_linear() doesn''t care about direction-flag?
This conditional assignment looks reversed.
done = reverse ? bytes_per_rep + (addr & ~PAGE_MASK) : -addr &
~PAGE_MASK;
Changing it fixes the legacy OS breakage i reported in a previous
private mail to Keir. Patch attached.
On Tue, Aug 19, 2008 at 7:41 PM, Keir Fraser <keir.fraser@eu.citrix.com>
wrote:> On 19/8/08 19:33, "Byrne, John (HP Labs)"
<john.l.byrne@hp.com> wrote:
>
>> 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.
>
> Yes, I considered giving that one a comment too. Now I will do so.
>
> -- Keir
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel