Re: [Xen-devel] [PATCH 1/3] [not-for-unstable] xen/arm: vgic-v3: Delay the initialization of the domain information

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/09/18 21:35, Julien Grall wrote:
>
>
> On 09/28/2018 12:11 AM, Stefano Stabellini wrote:
>> On Wed, 26 Sep 2018, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 09/25/2018 09:45 PM, Stefano Stabellini wrote:
>>>> On Tue, 4 Sep 2018, Andrew Cooper wrote:
>>>>> On 04/09/18 20:35, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 09/04/2018 08:21 PM, Julien Grall wrote:
>>>>>>> A follow-up patch will require to know the number of vCPUs when
>>>>>>> initializating the vGICv3 domain structure. However this
>>>>>>> information
>>>>>>> is
>>>>>>> not available at domain creation. This is only known once
>>>>>>> XEN_DOMCTL_max_vpus is called for that domain.
>>>>>>>
>>>>>>> In order to get the max vCPUs around, delay the domain part of the
>>>>>>> vGIC
>>>>>>> v3 initialization until the first vCPU of the domain is
>>>>>>> initialized.
>>>>>>>
>>>>>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>>>
>>>>>>> This is nasty but I can't find a better way for Xen 4.11 and older.
>>>>>>> This
>>>>>>> is not necessary for unstable as the number of vCPUs is known at
>>>>>>> domain
>>>>>>> creation.
>>>>>>>
>>>>>>> Andrew, I have CCed you to know whether you have a better idea
>>>>>>> where
>>>>>>> to
>>>>>>> place this call on Xen 4.11 and older.
>>>>>>
>>>>>> I just noticed that d->max_vcpus is initialized after
>>>>>> arch_domain_create. So without this patch on Xen 4.12, it will
>>>>>> not work.
>>>>>>
>>>>>> This is getting nastier because arch_domain_init is the one
>>>>>> initialize
>>>>>> the value returned by dom0_max_vcpus. So I am not entirely sure what
>>>>>> to do here.
>>>>>
>>>>> The positioning after arch_domain_create() is unfortunate, but I
>>>>> couldn’t manage better with ARM's current behaviour and Jan's
>>>>> insistence
>>>>> that the allocation of d->vcpu was common.  I'd prefer if the
>>>>> dependency
>>>>> could be broken and the allocation moved earlier.
>>>>>
>>>>> One option might be to have an arch_check_domainconfig() (or
>>>>> similar?)
>>>>> which is called very early on and can sanity check the values,
>>>>> including
>>>>> cross-checking the vgic and max_vcpus settings?  It could even be
>>>>> responsible for mutating XEN_DOMCTL_CONFIG_GIC_NATIVE into the
>>>>> correct
>>>>> real value.
>>>>>
>>>>> As for your patch here, its a gross hack, but its probably the best
>>>>> which can be done.
>>>>
>>>> *Sighs*
>>>> If that is what we have to do, it is as ugly as hell, but that is what
>>>> we'll do.
>>>
>>> This is the best we can do with the current code base. I think it
>>> would be
>>> worth reworking the code to make it nicer. I will add it in my TODO
>>> list.
>>>
>>>>
>>>> My only suggestion to marginally improve it would be instead of:
>>>>
>>>>> +    if ( v->vcpu_id == 0 )
>>>>> +    {
>>>>> +        rc = vgic_v3_real_domain_init(d);
>>>>> +        if ( rc )
>>>>> +            return rc;
>>>>> +    }
>>>>
>>>> to check on d->arch.vgic.rdist_regions instead:
>>>>
>>>>         if ( d->arch.vgic.rdist_regions == NULL )
>>>>         {
>>>>            // initialize domain
>>>
>>> I would prefer to keep v->vcpu_id == 0 just in case we end up to
>>> re-order the
>>> allocation in the future.
>>
>> I was suggesting to check on (rdist_regions == NULL) exactly for
>> potential re-ordering, in case in the future we end up calling
>> vcpu_vgic_init differently and somehow vcpu_init(vcpu1) is done before
>> before vcpu_init(vcpu0). Ideally we would like a way to check that
>> vgic_v3_real_domain_init has been called before and I thought
>> rdist_regions == NULL could do just that...
>
> What I meant by re-ordering is we manage to allocate the
> re-distributors before the vCPUs are created but still need
> vgic_v3_real_domain_init for other purpose.
>
> But vCPU initialization is potentially other issue.
>
> Anyway. both way have drawbacks. Yet I still prefer checking on the
> vCPU. It less likely vCPU0 will not be the first one initialized.

With the exception of the idle domain, all vcpus are strictly allocated
in packed ascending order.  Loads of other stuff will break if that
changed, so I wouldn't worry about it.

Furthermore, there is no obvious reason for this behaviour to ever change.

~Andrew

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