Re: [Xen-devel] [PATCH v3 3/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.07.16 at 17:51, <czuzu@xxxxxxxxxxxxxxx> wrote:
> @@ -492,8 +493,12 @@ int vcpu_initialise(struct vcpu *v)
>  
>  void vcpu_destroy(struct vcpu *v)
>  {
> -    xfree(v->arch.vm_event);
> -    v->arch.vm_event = NULL;
> +    if ( unlikely(v->arch.vm_event) )
> +    {
> +        xfree(v->arch.vm_event->emul_read_data);
> +        xfree(v->arch.vm_event);
> +        v->arch.vm_event = NULL;
> +    }

Considering the repeat of this pattern ...

> @@ -52,8 +58,25 @@ void vm_event_cleanup_domain(struct domain *d)
>  
>      for_each_vcpu ( d, v )
>      {
> -        xfree(v->arch.vm_event);
> -        v->arch.vm_event = NULL;
> +        if ( likely(!v->arch.vm_event) )
> +            continue;
> +
> +        /*
> +         * Only xfree the entire arch_vm_event if write_data was fully 
> handled.
> +         * Otherwise defer entire xfree until domain/vcpu destroyal.
> +         */
> +        if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
> +        {
> +            xfree(v->arch.vm_event->emul_read_data);
> +            xfree(v->arch.vm_event);
> +            v->arch.vm_event = NULL;
> +            continue;
> +        }

... here, please consider making this another helper (inline?) function.

> +        /* write_data not fully handled, only xfree emul_read_data */

Comment style again (and more below).

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v, 
> unsigned int port)
>  /* Clean up on domain destruction */
>  void vm_event_cleanup(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>  #ifdef CONFIG_HAS_MEM_PAGING
>      if ( d->vm_event->paging.ring_page )
>      {
> @@ -560,6 +562,16 @@ void vm_event_cleanup(struct domain *d)
>          (void)vm_event_disable(d, &d->vm_event->share);
>      }
>  #endif
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        if ( unlikely(v->arch.vm_event) )
> +        {
> +            /* vm_event->emul_read_data freed in vm_event_cleanup_domain */

Perhaps worthwhile adding a respective ASSERT()?

> +static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
> +{
> +    return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
> +}
> +
> +static inline bool_t vm_event_domain_initialised(struct domain *d)
> +{
> +    return (d->max_vcpus && d->vcpu[0] &&
> +            vm_event_vcpu_initialised(d->vcpu[0]));
> +}

Both functions' parameters should be const. Pointless parentheses
in both return statements.

Jan


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