Re: [Xen-devel] [PATCH V5 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 07/14/2015 04:37 PM, Jan Beulich wrote:
>>>> On 14.07.15 at 15:26, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 07/14/2015 03:22 PM, Jan Beulich wrote:
>>>>>> On 13.07.15 at 19:14, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -511,6 +511,8 @@ int vcpu_initialise(struct vcpu *v)
>>>>  
>>>>  void vcpu_destroy(struct vcpu *v)
>>>>  {
>>>> +    xfree(v->arch.vm_event.emul_read_data);
>>>
>>> Is this still needed now that you have
>>> vm_event_cleanup_domain()?
>>
>> I had thought that there might be a code path where
>> vm_event_cleanup_domain() doesn't get called and yet the domain is being
>> destroyed, but I can't find anything obvious in the code except a
>> comment in arch/x86/mm/shadow/common.c - shadow_final_teardown() -
>> stating that "It is possible for a domain that never got domain_kill()ed
>> to get here with its shadow allocation intact.".
> 
> Tim?
> 
>> Since common/domain.c's domain_kill() seems to be the only caller of
>> vm_event_cleanup(), I took that comment to mean that it could be
>> possible to end up in vcpu_destroy() without vm_event_cleanup_domain()
>> having been called, so I took the better-safe-than-sorry approach.
> 
> Better-safe-than-sorry would imply you'd also have to clear the
> pointer in vcpu_destroy(), covering the (hypothetical?) case of
> vm_event_cleanup_domain() being called afterwards.

True. If that can happen I'll clear the pointer there too.

>>>> +    {
>>>> +        unsigned int safe_bytes =
>>>> +            min(bytes, curr->arch.vm_event.emul_read_data->size);
>>>> +
>>>> +        if ( safe_bytes )
>>>> +            memcpy(buffer, curr->arch.vm_event.emul_read_data->data,
>>>> +                   safe_bytes);
>>>
>>> So why did you still keep this conditional?
>>
>> I thought a memcpy() call that ends up doing nothing (copying 0 bytes)
>> would be expensive and I've tried to optimize the code by not doing the
>> call if safe_bytes == 0.
> 
> That argumentation would then also apply to the subsequent
> memset().
> 
>> Since nobody else seems to think it's worth it, I'll remove it.
> 
> Thanks.
> 
>>>> @@ -1133,7 +1205,20 @@ static int hvmemul_rep_movs(
>>>>       */
>>>>      rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
>>>>      if ( rc == HVMCOPY_okay )
>>>> +    {
>>>> +        if ( unlikely(hvmemul_ctxt->set_context) )
>>>> +        {
>>>> +            rc = set_context_data(buf, bytes);
>>>> +
>>>> +            if ( rc != X86EMUL_OKAY)
>>>> +            {
>>>> +                xfree(buf);
>>>> +                return rc;
>>>> +            }
>>>> +        }
>>>> +
>>>>          rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
>>>> +    }
>>>
>>> Why do you not bypass hvm_copy_from_guest_phys() here? This
>>> way it would - afaict - become consistent with the other cases.
>>
>> You're right, it's unnecessary. Will remove the hvm_copy_from_guest_phys().
> 
> s/remove/bypass/ hopefully.

Yes, sorry for the typo.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel