Re: [Xen-devel] [PATCH RFC v1] x86/emulate: Send vm_event form emulate

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

>> +    if ( altp2m_active(current->domain) )
>> +        p2m = p2m_get_altp2m(current);
>> +    if ( !p2m )
>> +        p2m = p2m_get_hostp2m(current->domain);
>> +
>> +    gfn_lock(p2m, gfn, 0);
>> +    mfn = p2m->get_entry(p2m, gfn, &p2mt, &access, 0, NULL, NULL);
>> +    gfn_unlock(p2m, gfn, 0);
> 
> Don't you need to keep the gfn locked for at lest the duration of the
> function? Or else what you put in the req struct below might not be
> accurate by the time you write it. Maybe this is not relevant because
> this req ends up queued in an async ring, so by the time the other end
> reads the request the information might indeed have changed already.

I don't think the gfn should be locked for that long. I followed the 
model from p2m_mem_access_check() and there the gfn is locked in 
p2m_get_mem_access() only to get the access. This is relevant because 
the event is synced.
> 
> Also, I'm wondering whether there's no helper to fetch the gfn entry
> information from the p2m when using altp2m. The above construct (or
> variations of it) must be widely used in altp2m code?

I can change to p2m_get_mem_access() in the next version and lose some 
duplicated code here.
> 
>> +
>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>> +        return false;
>> +
>> +    switch ( access ) {
>> +    case p2m_access_x:
>> +    case p2m_access_rx:
>> +        if ( pfec & PFEC_write_access )
>> +            req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
>> +        break;
> 
> Newline.
> 
>> +    case p2m_access_w:
>> +    case p2m_access_rw:
>> +        if ( pfec & PFEC_insn_fetch )
>> +            req.u.mem_access.flags = MEM_ACCESS_X;
>> +        break;
> 
> Newline.
> 
>> +    case p2m_access_r:
>> +    case p2m_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;
> 
> Newline.
> 
>> +    default:
>> +        return false;
>> +    }
> 
> I'm not sure the switch is needed, you can't have a PFEC_write_access
> for example if the p2m type is p2m_access_w or p2m_access_rw, hence it
> seems like it could be simplified to only take the pfec into account?

It is possible to have PFEC_write_access and p2m_access_w but then there 
is no violation and the event will not be sent.

> 
>> +
>> +    if ( !req.u.mem_access.flags )
>> +        return false; //no violation
> 
> Wrong comment style.
> 

>> +            if ( hvmemul_send_vm_event(gpa, addr, gfn, pfec, hvmemul_ctxt) )
>> +            {
>> +                err = ERR_PTR(~X86EMUL_ACCESS_EXCEPTION);
>> +                goto out;
>> +            }
> 
> It seems to me that you could send the VM event in
> hvmemul_linear_to_phys directly instead of doing it in the caller. The
> same patter is seen below.

At this moment I send the event from 2 places and hvmemul_linear_to_phys 
is called from many other place so I don't think this will work.

> 
>> +
>>               if ( p2m_is_discard_write(p2mt) )
>>               {
>>                   err = ERR_PTR(~X86EMUL_OKAY);
>> @@ -694,96 +866,6 @@ static void hvmemul_unmap_linear_addr(
>>   #endif
>>   }
>>
>> -/*
>> - * Convert addr from linear to physical form, valid over the range
>> - * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
>> - * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
>> - * @pfec indicates the access checks to be performed during page-table 
>> walks.
>> - */
>> -static int hvmemul_linear_to_phys(
>> -    unsigned long addr,
>> -    paddr_t *paddr,
>> -    unsigned int bytes_per_rep,
>> -    unsigned long *reps,
>> -    uint32_t pfec,
>> -    struct hvm_emulate_ctxt *hvmemul_ctxt)
> 
> If you need to move code around, please do it in a pre-patch without
> introducing any change. That way it's much more easy for reviews to
> identify and review your code changes.

Sorry for this, I will split it up in v2 to have the main patch clear.

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