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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 17.09.2019 11:09, Jan Beulich wrote:
> 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?

Yes it should state "if the event is sent".

> 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

You are right, a check for HVMCOPY_linear should go in the if so we are 
sure that it is called with a linear address only.

> 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.

With this new check in place (HVMCOPY_linear) __hvm_copy() will be 
called from linear_read() linear_write() where it will pass down 
X86EMUL_RETRY.

The comment can change to:
"If a event is sent return paged_out. The emulation will have no side 
effect and will return X86EMUL_RETRY"

> 
>>>> @@ -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.
> 
It is safe enough to consider a invalid mfn from hostp2 to be a 
violation. There is still a small problem with having the altp2m view 
not having the page propagated from hostp2m. In this case we have to use
altp2m_get_effective_entry().

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