[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 Mon, Nov 19, 2018 at 03:56:14PM +0000, Alexandru Stefan ISAILA wrote: > > > 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. I would move the code that converts xenmem_access_t into a separate helper (as it's used in two different places), and use a bitmap instead of 3 boolean variables, so you can do: void convert_access(xenmem_access_t *access, unsigned int *attr) And don't need to repeat the switch in two different places. > > > >> > >>>> 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. I would just use hvm_emulate_one_vm_event if hvm_funcs.start_reexecute_instruction is unset, or else an explanation needs to be added to the commit message about why hvm_emulate_one_vm_event is not suitable. Also, after looking at the code I'm not sure I see why this needs to be VMX specific, AFAICT it doesn't directly call any VMX functions? Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |