On 07/14/2015 04:37 PM, Jan Beulich wrote:
>>>> On 14.07.15 at 15:26, <rcojoc...@bitdefender.com> wrote:
>> On 07/14/2015 03:22 PM, Jan Beulich wrote:
>>>>>> On 13.07.15 at 19:14, <rcojoc...@bitdefender.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
