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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2019 09:52, Alexandru Stefan ISAILA wrote:
> On 16.09.2019 18:58, Jan Beulich wrote:
>> On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy(
>>>               return HVMTRANS_bad_gfn_to_mfn;
>>>           }
>>>   
>>> +        if ( unlikely(v->arch.vm_event) &&
>>> +             v->arch.vm_event->send_event &&
>>> +             hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) )
>>> +        {
>>> +            put_page(page);
>>> +            return HVMTRANS_gfn_paged_out;
>>
>> I'm sorry, but there is _still_ no comment next to this apparent
>> mis-use of HVMTRANS_gfn_paged_out.
> 
> I will add this comment here:
> 
> "/*
>    * In case a vm event was sent return paged_out so the emulation will
>    * stop with no side effect
>    */"

First of all - why "was sent"? The event is yet to be sent, isn't it?
And then I'm afraid this still isn't enough. __hvm_copy() gets used
for many purposes. For example, while looking into this again when
preparing the reply here, I've noticed that above you may wrongly
call hvm_monitor_check_p2m() with npfec_kind_with_gla - there's no
linear address when HVMCOPY_linear is not set. If, while putting
together what the comment needs to explain (i.e. everything that
can't be implied from the code you add), you considered all cases
you should have noticed this yourself.

>>> @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, 
>>> unsigned int type,
>>>       monitor_traps(current, 1, &req);
>>>   }
>>>   
>>> +/*
>>> + * Send memory access vm_events based on pfec. Returns true if the event 
>>> was
>>> + * sent and false for p2m_get_mem_access() error, no violation and event 
>>> send
>>> + * error. Assumes the caller will check arch.vm_event->send_event.
>>> + *
>>> + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the 
>>> EPT
>>> + * (in which case access to it is unrestricted, so no violations can 
>>> occur).
>>> + * In this cases it is fine to continue the emulation.
>>> + */
>>
>> I think this part of the comment would better go ...
>>
>>> +bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t pfec,
>>> +                           uint16_t kind)
>>> +{
>>> +    xenmem_access_t access;
>>> +    vm_event_request_t req = {};
>>> +    paddr_t gpa = (gfn_to_gaddr(gfn) | (gla & ~PAGE_MASK));
>>> +
>>> +    ASSERT(current->arch.vm_event->send_event);
>>> +
>>> +    current->arch.vm_event->send_event = false;
>>> +
>>> +    if ( p2m_get_mem_access(current->domain, gfn, &access,
>>> +                            altp2m_vcpu_idx(current)) != 0 )
>>> +        return false;
>>
>> ... next to the call here (but the maintainers of the file would
>> have to judge in the end). That said, I continue to not understand
>> why a not found entry means unrestricted access. Isn't it
>> ->default_access which controls what such a "virtual" entry would
>> permit?
> 
> I'm sorry for this misleading comment. The code states that if entry was 
> not found the access will be default_access and return 0. So in this 
> case the default_access will be checked.
> 
> /* If request to get default access. */
> if ( gfn_eq(gfn, INVALID_GFN) )
> {
>      *access = memaccess[p2m->default_access];
>      return 0;
> }
> 
> If this clears thing up I can remove the "NOTE" part if the comment.

I'm afraid it doesn't clear things up: I'm still lost as to why
"entry not found" implies "full access". And I'm further lost as
to what the code fragment above (dealing with INVALID_GFN, but
not really the "entry not found" case, which would be INVALID_MFN
coming back from a translation) is supposed to tell me.

Jan

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