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
*/"
>
>> @@ -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.
>
>> + switch ( access )
>> + {
>> + case XENMEM_access_x:
>> + case XENMEM_access_rx:
>> + if ( pfec & PFEC_write_access )
>> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
>> + break;
>> +
>> + case XENMEM_access_w:
>> + case XENMEM_access_rw:
>> + if ( pfec & PFEC_insn_fetch )
>> + req.u.mem_access.flags = MEM_ACCESS_X;
>> + break;
>> +
>> + case XENMEM_access_r:
>> + case XENMEM_access_n:
>> + if ( pfec & PFEC_write_access )
>> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
>> + if ( pfec & PFEC_insn_fetch )
>> + req.u.mem_access.flags |= MEM_ACCESS_X;
>> + break;
>> +
>> + case XENMEM_access_wx:
>> + case XENMEM_access_rwx:
>> + case XENMEM_access_rx2rw:
>> + case XENMEM_access_n2rwx:
>> + case XENMEM_access_default:
>> + break;
>> + }
>> +
>> + if ( !req.u.mem_access.flags )
>> + return false; /* no violation */
>> +
>> + if ( kind == npfec_kind_with_gla )
>> + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA |
>> + MEM_ACCESS_GLA_VALID;
>> + else if ( kind == npfec_kind_in_gpt )
>> + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT |
>> + MEM_ACCESS_GLA_VALID;
>> +
>> +
>> + req.reason = VM_EVENT_REASON_MEM_ACCESS;
>> + req.u.mem_access.gfn = gfn_x(gfn);
>> + req.u.mem_access.gla = gla;
>> + req.u.mem_access.offset = gpa & ~PAGE_MASK;
>> +
>> + return monitor_traps(current, true, &req) >= 0;
>> +}
>
> There are quite a few uses of "current" in here - please consider
> latching into a local variable named "curr".
I will add a local variable here.
Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
