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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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