Re: [Xen-devel] [PATCH v11] x86/emulate: Send vm_event from emulate

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 20.09.2019 11:24, Jan Beulich wrote:
> On 20.09.2019 10:06, Alexandru Stefan ISAILA wrote:
>> On 19.09.2019 16:59, Jan Beulich wrote:
>>> Furthermore while you now restrict the check to linear address
>>> based accesses, other than the description says (or at least
>>> implies) you do not restrict it to read and exec accesses. It's
>>> not clear to me whether that's intentional, yet it affects which
>>> hvm_copy_*_linear() callers need auditing.
>>
>> The pfec var is checked in hvm_monitor_check_p2m(). If you think it is
>> necessary I can add one more check here for (pfec & (PFEC_insn_fetch |
>> PFEC_write_access)).
> 
> hvm_monitor_check_p2m() gets called from two places, so a check
> there won't help (afaict). The question whether to put an
> additional check here depends on whether, as the description
> says, you really only want to handle read/exec accesses here,
> or whether you also want to cover write ones (in which case the
> description should be adjusted so that it's not misleading).

Indeed covering write access here as well as in 
hvmemul_map_linear_addr() is needed. I will adjust the comment.

> 
>>> Finally, what about ->arch.vm_event->send_event remaining set
>>> after hvm_emulate_one_vm_event(), because hvm_monitor_check_p2m()
>>> (the only place where the flag would get cleared) was never hit
>>> in the process?
>> Thanks for pointing this out, indeed it's a problem here. A solution can
>> be to move send_event = false; after hvm_emulate_one_vm_event() is
>> finished. And state in the comment that the user is in charge of
>> enabling and disabling the flag.
>> Or just have it in both places.
> 
> For this aspect alone I think you want it in both places, but ...
> 
>>> And what about an instruction accessing two (or
>>> more) distinct addresses? The flag would be clear after the first
>>> one was checked afaict.
>>
>> There is no problem here because emulation will stop after the first bad
>> access so there is no need to continue.
> 
> ... for this moving it may indeed be necessary. I have to admit
> that I don't follow your reply here: The flag will also be clear
> after the first good access (afaict), and hence further accesses
> by the same insn won't make it into hvm_monitor_check_p2m() at all.
> 

Ok I will move it from hvm_monitor_check_p2m() and adjust the comment 
accordingly.


Thanks for the review,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel