Re: [Xen-devel] [PATCH for-next 6/8] x86/domain: factor out pv_domain_initialise

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.04.17 at 15:27, <wei.liu2@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -533,6 +533,58 @@ static void pv_domain_destroy(struct domain *d)
>      free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
>  }
>  
> +static int pv_domain_initialise(struct domain *d, unsigned int domcr_flags)
> +{
> +    static const struct arch_csw pv_csw = {
> +        .from = paravirt_ctxt_switch_from,
> +        .to   = paravirt_ctxt_switch_to,
> +        .tail = continue_nonidle_domain,
> +    };
> +    int rc = -ENOMEM;
> +
> +    d->arch.pv_domain.gdt_ldt_l1tab =
> +        alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
> +    if ( !d->arch.pv_domain.gdt_ldt_l1tab )
> +        goto fail;
> +    clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
> +
> +    if ( levelling_caps & ~LCAP_faulting )
> +    {
> +        d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
> +        if ( !d->arch.pv_domain.cpuidmasks )
> +            goto fail;
> +        *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
> +    }
> +
> +    rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> +                                  GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> +                                  NULL, NULL);
> +    if ( rc )
> +        goto fail;
> +
> +    d->arch.ctxt_switch = &pv_csw;
> +
> +    /* 64-bit PV guest by default. */
> +    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +
> +    return 0;
> +
> +fail:

Labels indented by at least one space please.

> +    if ( d->arch.pv_domain.gdt_ldt_l1tab )
> +    {
> +        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> +        d->arch.pv_domain.gdt_ldt_l1tab = NULL;
> +    }
> +
> +    if ( d->arch.pv_domain.cpuidmasks )
> +    {
> +        xfree(d->arch.pv_domain.cpuidmasks);
> +        d->arch.pv_domain.cpuidmasks = NULL;
> +    }

Perhaps better to amend pv_domain_destroy() with the two NULL
assignments, and call it from here?

> @@ -593,30 +645,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>          d->arch.emulation_flags = emflags;
>      }
>  
> -    if ( is_idle_domain(d) )
> -        rc = 0;
> -    else
> -    {
> -        d->arch.pv_domain.gdt_ldt_l1tab =
> -            alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
> -        if ( !d->arch.pv_domain.gdt_ldt_l1tab )
> -            goto fail;
> -        clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
> -
> -        if ( levelling_caps & ~LCAP_faulting )
> -        {
> -            d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
> -            if ( !d->arch.pv_domain.cpuidmasks )
> -                goto fail;
> -            *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
> -        }
> -
> -        rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> -                                      GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> -                                      NULL, NULL);
> -    }
> -    if ( rc )
> -        goto fail;
> +    rc = 0;

Seems to point out that the latest now the initializer of the variable
is pointless (and therefore could be switched to 0 instead of this
assignment, if an initializer is needed at all - not sure about that).

Jan


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