[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1] x86/hvm: Generic instruction re-execution mechanism for execute faults
On 19.11.2018 17:08, Roger Pau Monné wrote: > On Mon, Nov 19, 2018 at 01:30:09PM +0000, Alexandru Stefan ISAILA wrote: >>>> + /* Now transform our RWX values in a XENMEM_access_* constant. */ >>>> + if ( r == 0 && w == 0 && x == 0 ) >>>> + new_access = XENMEM_access_n; >>>> + else if ( r == 0 && w == 0 && x == 1 ) >>>> + new_access = XENMEM_access_x; >>>> + else if ( r == 0 && w == 1 && x == 0 ) >>>> + new_access = XENMEM_access_w; >>>> + else if ( r == 0 && w == 1 && x == 1 ) >>>> + new_access = XENMEM_access_wx; >>>> + else if ( r == 1 && w == 0 && x == 0 ) >>>> + new_access = XENMEM_access_r; >>>> + else if ( r == 1 && w == 0 && x == 1 ) >>>> + new_access = XENMEM_access_rx; >>>> + else if ( r == 1 && w == 1 && x == 0 ) >>>> + new_access = XENMEM_access_rw; >>>> + else if ( r == 1 && w == 1 && x == 1 ) >>>> + new_access = XENMEM_access_rwx; >>>> + else >>>> + new_access = required_access; /* Should never get here. */ >>> >>> There seems to be a lot of translation from xenmem_access_t to bool >>> fields and then to xenmem_access_t again. Can't you just avoid the >>> booleans? >> >> The translation is done because the rights are cumulative and I think >> this is the clear way to do this. > > So the switch converts required_access using the following relation: > > _r -> r = 1 w = 0 x = 0 > _w -> r = 0 w = 1 x = 0 > _x -> r = 0 w = 0 x = 1 > _rx -> r = 1 w = 1 x = 0 > _wx -> r = 0 w = 1 x = 1 > _rw -> r = 1 w = 1 x = 0 > _rwx -> r = 1 w = 1 x = 1 > > Then the if below performs the following transformation: > > r = 0 w = 0 x = 0 -> _n > r = 1 w = 0 x = 0 -> _r > r = 0 w = 1 x = 0 -> _w > r = 0 w = 0 x = 1 -> _x > r = 1 w = 1 x = 0 -> _rw > r = 0 w = 1 x = 1 -> _wx > r = 1 w = 1 x = 0 -> _rw > r = 1 w = 1 x = 1 -> _rwx > > I'm not sure I understand this chunk of code, because you end up > getting exactly the same type that you have as the input, and a type > not listed here is just silently passed through, so I don't see the > point in doing this transformation. The first switch is for cur_access and it sets r,w,x accordingly, the second switch is required_access where r,w,x are appended and then in the last if().. part new_access is assigned according to the previous assignments of r,w,x. > >> >>>> if ( vm_event_check_ring(d->vm_event_monitor) && >>>> d->arch.monitor.inguest_pagefault_disabled && >>>> - npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */ >>>> + npfec.kind != npfec_kind_with_gla && >>>> + hvm_funcs.start_reexecute_instruction ) /* don't send a >>>> mem_event */ >>>> { >>>> - hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, >>>> X86_EVENT_NO_EC); >>>> - >>>> + v->arch.vm_event->emulate_flags = 0; >>>> + hvm_funcs.start_reexecute_instruction(v, gpa, XENMEM_access_rw); >>>> return true; >>>> } >>> >>> Don't you need to fallback to using hvm_emulate_one_vm_event if >>> start_reexecute_instruction is not available? >> >> Fallback with hvm_emulate_one_vm_event can result in loosing events. > > But by changing this here unconditionally you are removing this > functionality on AMD hardware, which it used to have before by making > use of hvm_emulate_one_vm_event. > > I think this needs to at least be written in the commit message. For AMD I could add if (cpu_has_svm()) and call emulate_one_vm_event. Introspection wise loosing valuable info is not a good thing to have because it could result in security breach. > >>>> + for_each_vcpu(d, v) >>>> + { >>>> + if ( !v->arch.rexec_level ) >>>> + continue; >>>> + >>>> + for ( i = v->arch.rexec_level - 1; i >= 0; i-- ) >>> >>> Is there any reason this has to be done backwards? >>> >>> If you do it from 0 to v->arch.rexec_level you could use an unsigned >>> int as the index. >> >> This is done backwards because of the corresponding code in >> vmx_stop_reexecute_instruction() but here it can be turned the other way >> if you insist on i to be unsigned. > > Yes, Jan has also suggested a way to make i unsigned while keeping the > loop backwards, but I don't see the point of performing the loop > backwards if there's no need. > There is no problem here, I can change it in the next version. Regards, Alex _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |