[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Andrew,

On 10/01/2018 10:53 AM, Andrew Cooper wrote:
On 01/10/18 10:43, Julien Grall wrote:
Hi,

On 09/29/2018 12:48 AM, Andrew Cooper wrote:
On 29/09/18 00:45, Stefano Stabellini wrote:
On Sat, 29 Sep 2018, Andrew Cooper wrote:
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.
OK, let's go with Julien's patch. We need a new tag for this, something
like:

Acked-but-disliked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

Do bear in mind that this patch is only for 4.11 and earlier.  I've
already fixed staging (i.e. 4.12) when it comes to knowing
d->max_vcpus :)
I thought we agreed that patch is necessary for 4.12 as d->max_vcpus
is initialized after arch_domain_init?

Oh right.

I am not planning to do the rework in short term. Did you do more work
on around domain_create recently?

There are multiple related patch series out on xen-devel atm, but I
expect I need to spin a new version of each of them.  I'll see if I have
some time to put towards it.  Are you happy in principle with the
arch_check_domainconfig() plan?

I am happy in principle. If you don't have time to work on it, I will try to have a look before Xen 4.12.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.