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

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On 01.07.2019 17:55, Jan Beulich wrote:
> On 01.07.2019 16:45, Alexandru Stefan ISAILA wrote:
>> On 01.07.2019 16:13, Jan Beulich wrote:
>>>>>> On 04.06.19 at 13:49, <aisaila@bitdefender.com> wrote:
>>>> +    if ( !send_event || !pfec )
>>>> +        return false;
>>>
>>> I think I've said before that the !pfec part need an explanation (in
>>> a comment). Without such an explanation I'm inclined to say it
>>> should be deleted. If otoh this is simply mean to be a shortcut,
>>> then you should really just check the two bits you actually care
>>> about further down.
>>
>> The pfec check is done because I encountered pfec 0 in tests. It could
>> save some work if pfec == 0 when calling the function. Is there
>> a must in having this check removed? If so then it can be done. The
>> send_event will be checked before calling the function as you said.
> 
> It's not a requirement for it to be removed, _if_ there's a good
> reason for it to be there. I don't, however, see how pfec=0 could
> be a problem - afaict it would return false a few lines further
> down in that case.

You are right, pfec=0 wold not be a problem and it will be caught in the 
no violation if.

> 
>>>> +    if ( !req.u.mem_access.flags )
>>>> +        return false; /* no violation */
>>>
>>> How is the "false" here (I think this is the one the description talks
>>> about) matching up with the various other ones in the function?
>>
>> There should be no event if there is no access violation. So in this
>> case the emulation is continued as expected.
> 
> But this doesn't answer my question: You use "false" as return value
> to indicate different things. Only the one here means "no access
> violation".

Sorry about that, since this will remain the only return false apart 
form the main one (return monitor_traps()), false  = event was not sent 
and true = event was sent.

I understand that you are asking about the scenario when there was a 
violation and the event was not sent. Then I can issue a domain_crash() 
as that is potentially a big issue.

I hope I got that correctly.

> 
>>>> @@ -615,6 +669,13 @@ static void *hvmemul_map_linear_addr(
>>>>     
>>>>             if ( pfec & PFEC_write_access )
>>>>             {
>>>> +            if ( hvm_emulate_send_vm_event(addr, gfn, pfec,
>>>> +                                           hvmemul_ctxt->send_event) )
>>>> +            {
>>>> +                err = ERR_PTR(~X86EMUL_RETRY);
>>>> +                goto out;
>>>> +            }
>>>
>>> How come this sits only on the write path?
>>
>> We are interested only for the write access on this path. This also
>> ensures that pfec is set.
> 
> I'm sorry, but the event sending should not be tailored to what you
> need or want. Or if so (i.e. if agreed upon among the VM event
> maintainers) then this restriction should be clearly spelled out.
> 

On the other hand, this can go outside the write path with no effect on 
the functionality of this send_event feature.

I will move it after the if(write) in the next version.


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