Re: [Xen-devel] [PATCH RFC v1 54/74] xen/pvshim: set correct domid value

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> @@ -94,6 +95,24 @@ void __init pv_shim_setup_dom(struct domain *d, 
> l4_pgentry_t *l4start,
>  #undef SET_AND_MAP_PARAM
>  }
>  
> +void pv_shim_shutdown(uint8_t reason)
> +{
> +    /* XXX: handle suspend */
> +    xen_hypercall_shutdown(reason);
> +}

Does this really need to be an out-of-line function? But yes, the
todo item probably warrants it.

> +domid_t get_dom0_domid(void)

What a strange name - to me Dom0's domain ID can only ever be
zero.

> +{
> +    uint32_t eax, ebx, ecx, edx;
> +
> +    if ( !pv_shim )
> +        return 0;
> +
> +    cpuid(hypervisor_cpuid_base() + 1, &eax, &ebx, &ecx, &edx);
> +
> +    return ebx ?: 1;
> +}

Not having another way to obtain the domain ID, returning 1
here is nevertheless dangerous in case the client domain actually
means to use its domain ID instead of DOMID_SELF anywhere. At
the very least this should be stated clearly in the description
(serving as a hint that the CPUID change should be backported
by anyone wanting to use the shim on their hypervisors).

> @@ -576,11 +578,11 @@ static void noinline init_done(void)
>  
>      system_state = SYS_STATE_active;
>  
> +    domain_unpause_by_systemcontroller(dom0);
> +
>      /* MUST be done prior to removing .init data. */
>      unregister_init_virtual_region();
>  
> -    domain_unpause_by_systemcontroller(hardware_domain);

Why the re-ordering? Along the lines of the earlier comment,
using "dom0" as replacement (static) variable isn't very nice.
Please at least accompany its declaration by a comment.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel