[PATCH 0/3] xen/arm: vgic-v3: Bug fixes

[PATCH 0/3] xen/arm: vgic-v3: Bug fixes

[Julien Grall]

Hi all,

The first 2 patches of the series are meant to address Dom0 boot failure
when on GICv4 platforms as well as when the number of vCPUs is not equal to
the numbers of pCPUs. They should be backported up Xen 4.8.

Patch #3 should address failure when failing to create guest. ITS is still
under EXPERT, so I would not consider it as backport.

Cheers,

Julien Grall (3):
  [not-for-unstable] xen/arm: vgic-v3: Delay the initialization of the
    domain information
  xen/arm: vgic-v3: Don't create empty re-distributor regions
  xen/arm: vgic-v3-its: Make vgic_v3_its_free_domain idempotent

 xen/arch/arm/gic-v3.c      | 10 ++++++++--
 xen/arch/arm/vgic-v3-its.c |  4 ++++
 xen/arch/arm/vgic-v3.c     | 40 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 4 deletions(-)

-- 
2.11.0


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

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

[Julien Grall]

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@arm.com>

---

Cc: Andrew Cooper <andrew.cooper3@citrix.com>

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.
---
 xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 4b42739a52..df1bab3a35 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1573,9 +1573,11 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
     .write = vgic_v3_distr_mmio_write,
 };
 
+static int vgic_v3_real_domain_init(struct domain *d);
+
 static int vgic_v3_vcpu_init(struct vcpu *v)
 {
-    int i;
+    int i, rc;
     paddr_t rdist_base;
     struct vgic_rdist_region *region;
     unsigned int last_cpu;
@@ -1584,6 +1586,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     struct domain *d = v->domain;
 
     /*
+     * This is the earliest place where the number of vCPUs is
+     * known. This is required to initialize correctly the vGIC v3
+     * domain structure. We only to do that when vCPU 0 is
+     * initilialized.
+     */
+    if ( v->vcpu_id == 0 )
+    {
+        rc = vgic_v3_real_domain_init(d);
+        if ( rc )
+            return rc;
+    }
+
+    /*
      * Find the region where the re-distributor lives. For this purpose,
      * we look one region ahead as we have only the first CPU in hand.
      */
@@ -1641,7 +1656,7 @@ static inline unsigned int vgic_v3_rdist_count(struct domain *d)
                GUEST_GICV3_RDIST_REGIONS;
 }
 
-static int vgic_v3_domain_init(struct domain *d)
+static int vgic_v3_real_domain_init(struct domain *d)
 {
     struct vgic_rdist_region *rdist_regions;
     int rdist_count, i, ret;
@@ -1733,6 +1748,16 @@ static int vgic_v3_domain_init(struct domain *d)
     return 0;
 }
 
+static int vgic_v3_domain_init(struct domain *d)
+{
+    /*
+     * The domain initialization for vGIC v3 is delayed until the first vCPU
+     * is created. This because the initialization may require to know the
+     * number of vCPUs that is not known when creating the domain.
+     */
+    return 0;
+}
+
 static void vgic_v3_domain_free(struct domain *d)
 {
     vgic_v3_its_free_domain(d);
-- 
2.11.0


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

[PATCH 2/3] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

At the moment, Xen is assuming the hardware domain will have the same
number of re-distributor regions as the host. However, as the
number of CPUs or the stride (e.g on GICv4) may be different we end up
exposing regions which does not contain any re-distributors.

When booting, Linux will go through all the re-distributor region to
check whether a property (e.g vPLIs) is available accross all the
re-distributors. This will result to a data abort on empty regions
because there are no underlying re-distributor.

So we need to limit the number of regions exposed to the hardware
domain. The code reworked to only expose the minimun number of regions
required by the hardware domain. It is assumed the regions will be
populated starting from the first one.

Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v3.c  | 10 ++++++++--
 xen/arch/arm/vgic-v3.c | 11 +++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b2ed0f8b55..4a984cfb12 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1274,8 +1274,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
      * GIC has two memory regions: Distributor + rdist regions
      * CPU interface and virtual cpu interfaces accessesed as System registers
      * So cells are created only for Distributor and rdist regions
+     * The hardware domain may not used all the regions. So only copy
+     * what is necessary.
      */
-    new_len = new_len * (gicv3.rdist_count + 1);
+    new_len = new_len * (d->arch.vgic.nr_regions + 1);
 
     hw_reg = dt_get_property(gic, "reg", &len);
     if ( !hw_reg )
@@ -1503,7 +1505,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 
     /* Add Generic Redistributor */
     size = sizeof(struct acpi_madt_generic_redistributor);
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    /*
+     * The hardware domain may not used all the regions. So only copy
+     * what is necessary.
+     */
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
     {
         gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
         gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index df1bab3a35..9f729862da 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1695,8 +1695,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
             d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
 
             first_cpu += size / GICV3_GICR_SIZE;
+
+            if ( first_cpu >= d->max_vcpus )
+                break;
         }
 
+        /*
+         * The hardware domain may not used all the re-distributors
+         * regions (e.g when the number of vCPUs does not match the
+         * number of pCPUs). Update the number of regions to avoid
+         * exposing unused region as they will not get emulated.
+         */
+        d->arch.vgic.nr_regions = i + 1;
+
         d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
     }
     else
-- 
2.11.0


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

[PATCH 3/3] xen/arm: vgic-v3-its: Make vgic_v3_its_free_domain idempotent

[Julien Grall]

vgic_v3_its_free_domain may be called before vgic_v3_its_init_domain if
the vGIC was failing to initalize itself. This means the list would be
unitialized and result in a crash.

Thankfully, we only allow ITS for the hardware domain. So the crash is
not a security issue. Fix it by checking whether the list the NULL.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/arm/vgic-v3-its.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 32061c6b03..9edd97c4e7 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1548,6 +1548,10 @@ void vgic_v3_its_free_domain(struct domain *d)
 {
     struct virt_its *pos, *temp;
 
+    /* Cope with unitialized vITS */
+    if ( list_head_is_null(&d->arch.vgic.vits_list) )
+        return;
+
     list_for_each_entry_safe( pos, temp, &d->arch.vgic.vits_list, vits_list )
     {
         list_del(&pos->vits_list);
-- 
2.11.0


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

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

[Julien Grall]

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@arm.com>
> 
> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> 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.

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

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@arm.com>
>>
>> ---
>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> 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.  I have to admit that I'm surprised/concerned that
Xen has lasted this long with such a fundamental gap in domain creation.

~Andrew

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

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

[Julien Grall]

Hi Andrew,

On 09/04/2018 08:53 PM, 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@arm.com>
>>>
>>> ---
>>>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> 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.

I think it is doable without too much trouble on Arm. I can have a look 
at this for Xen 4.12 once this series is merged.

> 
> As for your patch here, its a gross hack, but its probably the best
> which can be done.  I have to admit that I'm surprised/concerned that
> Xen has lasted this long with such a fundamental gap in domain creation.

I am surprised too. Hopefully your rework will make everything easier to 
use.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 0/3] xen/arm: vgic-v3: Bug fixes

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 04 September 2018 20:22
> To: xen-devel@lists.xenproject.org
> Cc: sstabellini@kernel.org; andre.przywara@arm.com; Shameerali Kolothum
> Thodi <shameerali.kolothum.thodi@huawei.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: [PATCH 0/3] xen/arm: vgic-v3: Bug fixes
> 
> Hi all,
> 
> The first 2 patches of the series are meant to address Dom0 boot failure when
> on GICv4 platforms as well as when the number of vCPUs is not equal to the
> numbers of pCPUs. They should be backported up Xen 4.8.

Tested first two patches on this series on ARM64 GICv4 platform(ACPI boot) and
the reported issue[1] is no more present. Please feel free to add,

Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Thanks,
Shameer

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg00138.html

> Patch #3 should address failure when failing to create guest. ITS is still under
> EXPERT, so I would not consider it as backport.
> 
> Cheers,
> 
> Julien Grall (3):
>   [not-for-unstable] xen/arm: vgic-v3: Delay the initialization of the
>     domain information
>   xen/arm: vgic-v3: Don't create empty re-distributor regions
>   xen/arm: vgic-v3-its: Make vgic_v3_its_free_domain idempotent
> 
>  xen/arch/arm/gic-v3.c      | 10 ++++++++--
>  xen/arch/arm/vgic-v3-its.c |  4 ++++
>  xen/arch/arm/vgic-v3.c     | 40 ++++++++++++++++++++++++++++++++++++++-
> -
>  3 files changed, 50 insertions(+), 4 deletions(-)
> 
> --
> 2.11.0


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

Re: [PATCH 3/3] xen/arm: vgic-v3-its: Make vgic_v3_its_free_domain idempotent

[Stefano Stabellini]

On Tue, 4 Sep 2018, Julien Grall wrote:
> vgic_v3_its_free_domain may be called before vgic_v3_its_init_domain if
> the vGIC was failing to initalize itself. This means the list would be
> unitialized and result in a crash.
> 
> Thankfully, we only allow ITS for the hardware domain. So the crash is
> not a security issue. Fix it by checking whether the list the NULL.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/arm/vgic-v3-its.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 32061c6b03..9edd97c4e7 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1548,6 +1548,10 @@ void vgic_v3_its_free_domain(struct domain *d)
>  {
>      struct virt_its *pos, *temp;
>  
> +    /* Cope with unitialized vITS */
> +    if ( list_head_is_null(&d->arch.vgic.vits_list) )
> +        return;
> +
>      list_for_each_entry_safe( pos, temp, &d->arch.vgic.vits_list, vits_list )
>      {
>          list_del(&pos->vits_list);
> -- 
> 2.11.0
> 

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

Re: [PATCH 2/3] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Stefano Stabellini]

On Tue, 4 Sep 2018, Julien Grall wrote:
> At the moment, Xen is assuming the hardware domain will have the same
> number of re-distributor regions as the host. However, as the
> number of CPUs or the stride (e.g on GICv4) may be different we end up
> exposing regions which does not contain any re-distributors.
> 
> When booting, Linux will go through all the re-distributor region to
> check whether a property (e.g vPLIs) is available accross all the
> re-distributors. This will result to a data abort on empty regions
> because there are no underlying re-distributor.
> 
> So we need to limit the number of regions exposed to the hardware
> domain. The code reworked to only expose the minimun number of regions
> required by the hardware domain. It is assumed the regions will be
> populated starting from the first one.

I have a question: given that it is possible for a rdist_region to cover
more than 1 cpu, could we get into troubles if the last rdist_region of
the hardware_domain covers 2 cpus, but actually dom0 only has 1 vcpu?
get_vcpu_from_rdist would return NULL and vgic_v3_rdistr_mmio_read/write
would return 0. This case seems to be handled correctly but I wanted to
double check.


I think we also need to fix vgic_v3_rdist_count? Today it just returns
vgic_v3_hw.nr_rdist_regions for dom0. It would be bad if we left it
unfixed? If we do that, we might be able to get rid of the modifications
to vgic_v3_real_domain_init.


> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/gic-v3.c  | 10 ++++++++--
>  xen/arch/arm/vgic-v3.c | 11 +++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index b2ed0f8b55..4a984cfb12 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1274,8 +1274,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       * GIC has two memory regions: Distributor + rdist regions
>       * CPU interface and virtual cpu interfaces accessesed as System registers
>       * So cells are created only for Distributor and rdist regions
> +     * The hardware domain may not used all the regions. So only copy
> +     * what is necessary.
>       */
> -    new_len = new_len * (gicv3.rdist_count + 1);
> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);

Do we also need to fix "#redistributor-regions" just above?


>      hw_reg = dt_get_property(gic, "reg", &len);
>      if ( !hw_reg )
> @@ -1503,7 +1505,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>  
>      /* Add Generic Redistributor */
>      size = sizeof(struct acpi_madt_generic_redistributor);
> -    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    /*
> +     * The hardware domain may not used all the regions. So only copy
                                      ^ use


> +     * what is necessary.
> +     */
> +    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
>      {
>          gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
>          gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index df1bab3a35..9f729862da 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1695,8 +1695,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
>              d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
>  
>              first_cpu += size / GICV3_GICR_SIZE;
> +
> +            if ( first_cpu >= d->max_vcpus )
> +                break;

This is just a matter of code style and preferences, but I would prefer
if the termination condition was at the top as part of the for
statement. Of course, it works regardless, so the patch would be
OK either way.



>          }
>  
> +        /*
> +         * The hardware domain may not used all the re-distributors
                                          ^ use


> +         * regions (e.g when the number of vCPUs does not match the
> +         * number of pCPUs). Update the number of regions to avoid
> +         * exposing unused region as they will not get emulated.
                               ^ regions


> +         */
> +        d->arch.vgic.nr_regions = i + 1;
>          d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
>      }
>      else

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2407 bytes --]

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@arm.com>
> >>
> >> ---
> >>
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>
> >> 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.

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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Julien Grall]

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@arm.com>
>>>>
>>>> ---
>>>>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>> 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.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/3] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

Hi Stefano,

On 09/25/2018 09:38 PM, Stefano Stabellini wrote:
> On Tue, 4 Sep 2018, Julien Grall wrote:
>> At the moment, Xen is assuming the hardware domain will have the same
>> number of re-distributor regions as the host. However, as the
>> number of CPUs or the stride (e.g on GICv4) may be different we end up
>> exposing regions which does not contain any re-distributors.
>>
>> When booting, Linux will go through all the re-distributor region to
>> check whether a property (e.g vPLIs) is available accross all the
>> re-distributors. This will result to a data abort on empty regions
>> because there are no underlying re-distributor.
>>
>> So we need to limit the number of regions exposed to the hardware
>> domain. The code reworked to only expose the minimun number of regions
>> required by the hardware domain. It is assumed the regions will be
>> populated starting from the first one.
> 
> I have a question: given that it is possible for a rdist_region to cover
> more than 1 cpu, could we get into troubles if the last rdist_region of
> the hardware_domain covers 2 cpus, but actually dom0 only has 1 vcpu?
> get_vcpu_from_rdist would return NULL and vgic_v3_rdistr_mmio_read/write
> would return 0.
> This case seems to be handled correctly but I wanted to
> double check.

0 means a data abort will be injected into the guest. However, the guest 
should never touch that because the last valid re-distributor of the 
regions will have GICR_TYPER.Last set.

So the guest OS will stop looking for more re-distributors in that region.

>  >
> I think we also need to fix vgic_v3_rdist_count? Today it just returns
> vgic_v3_hw.nr_rdist_regions for dom0. It would be bad if we left it
> unfixed? If we do that, we might be able to get rid of the modifications
> to vgic_v3_real_domain_init.

We don't want to fix vgic_v3_rdist_count. The helper returns the maximum 
re-distributors regions. This is used to compute the number of IO 
handlers and allocating the array storing the regions.

I am pretty sure you will say we will waste memory. However, at the 
moment,  we need to know the number of IO handlers much before we know 
the number of vCPUs. For the array, we would need to go through the 
regions twice (regions may not be the same size so we can't infer easily 
the number needed). Overall, the amount of memory used is the same as 
today (so not really a waste per-se).

It might be possible to limit that once we reworked the common code to 
know the number of vCPUs earlier on (see discussion on patch #1).

>> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/gic-v3.c  | 10 ++++++++--
>>   xen/arch/arm/vgic-v3.c | 11 +++++++++++
>>   2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index b2ed0f8b55..4a984cfb12 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1274,8 +1274,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>>        * GIC has two memory regions: Distributor + rdist regions
>>        * CPU interface and virtual cpu interfaces accessesed as System registers
>>        * So cells are created only for Distributor and rdist regions
>> +     * The hardware domain may not used all the regions. So only copy
>> +     * what is necessary.
>>        */
>> -    new_len = new_len * (gicv3.rdist_count + 1);
>> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
> 
> Do we also need to fix "#redistributor-regions" just above?
Hmm I missed that one. Not sure why it didn't show up in my test.

>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index df1bab3a35..9f729862da 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -1695,8 +1695,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
>>               d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
>>   
>>               first_cpu += size / GICV3_GICR_SIZE;
>> +
>> +            if ( first_cpu >= d->max_vcpus )
>> +                break;
> 
> This is just a matter of code style and preferences, but I would prefer
> if the termination condition was at the top as part of the for
> statement. Of course, it works regardless, so the patch would be
> OK either way.

I thought about it when writing this patch. This would look like:

for ( i = 0;
      (i < vgic_v3_hw.nr_dist_regions) && (first_cpu < d->max_vcpus);
      i++ )

This is IHMO more difficult to understand (long condition) and slightly 
strange because for is not incrementing directly first_cpu.

I will stick with the current implementation unless you have a more 
readable solution.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3501 bytes --]

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@arm.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > > 
> > > > > 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...

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 2/3] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Stefano Stabellini]

On Wed, 26 Sep 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/25/2018 09:38 PM, Stefano Stabellini wrote:
> > On Tue, 4 Sep 2018, Julien Grall wrote:
> > > At the moment, Xen is assuming the hardware domain will have the same
> > > number of re-distributor regions as the host. However, as the
> > > number of CPUs or the stride (e.g on GICv4) may be different we end up
> > > exposing regions which does not contain any re-distributors.
> > > 
> > > When booting, Linux will go through all the re-distributor region to
> > > check whether a property (e.g vPLIs) is available accross all the
> > > re-distributors. This will result to a data abort on empty regions
> > > because there are no underlying re-distributor.
> > > 
> > > So we need to limit the number of regions exposed to the hardware
> > > domain. The code reworked to only expose the minimun number of regions
> > > required by the hardware domain. It is assumed the regions will be
> > > populated starting from the first one.
> > 
> > I have a question: given that it is possible for a rdist_region to cover
> > more than 1 cpu, could we get into troubles if the last rdist_region of
> > the hardware_domain covers 2 cpus, but actually dom0 only has 1 vcpu?
> > get_vcpu_from_rdist would return NULL and vgic_v3_rdistr_mmio_read/write
> > would return 0.
> > This case seems to be handled correctly but I wanted to
> > double check.
> 
> 0 means a data abort will be injected into the guest. However, the guest
> should never touch that because the last valid re-distributor of the regions
> will have GICR_TYPER.Last set.
> 
> So the guest OS will stop looking for more re-distributors in that region.

OK


> >  >
> > I think we also need to fix vgic_v3_rdist_count? Today it just returns
> > vgic_v3_hw.nr_rdist_regions for dom0. It would be bad if we left it
> > unfixed? If we do that, we might be able to get rid of the modifications
> > to vgic_v3_real_domain_init.
> 
> We don't want to fix vgic_v3_rdist_count. The helper returns the maximum
> re-distributors regions.

We don't want to or we can't? Because it looks like we would want to fix
vgic_v3_rdist_count if we could, right? It is called from domain
specific initialization functions, so theoretically it should return
domain specific vgic information, not physical information.


> This is used to compute the number of IO handlers and
> allocating the array storing the regions.
> 
> I am pretty sure you will say we will waste memory. However, at the moment,
> we need to know the number of IO handlers much before we know the number of
> vCPUs. For the array, we would need to go through the regions twice (regions
> may not be the same size so we can't infer easily the number needed). Overall,
> the amount of memory used is the same as today (so not really a waste per-se).
> 
> It might be possible to limit that once we reworked the common code to know
> the number of vCPUs earlier on (see discussion on patch #1).

Yeah, this is nasty, but it is clear that until we rework the code to
set d->max_vcpus earlier it won't get fixed. Nothing we can do here.

So, I think ideally we would want to fix vgic_v3_rdist_count, but today
we can't. Maybe we could rename vgic_v3_rdist_count to
vgic_v3_hw_rdist_count, and add a short TODO comment somewhere in the
file?

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

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

[Julien Grall]

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@arm.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>
>>>>>> 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.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/3] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

On 09/28/2018 12:34 AM, Stefano Stabellini wrote:
> On Wed, 26 Sep 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/25/2018 09:38 PM, Stefano Stabellini wrote:
>>> On Tue, 4 Sep 2018, Julien Grall wrote:
>>>> At the moment, Xen is assuming the hardware domain will have the same
>>>> number of re-distributor regions as the host. However, as the
>>>> number of CPUs or the stride (e.g on GICv4) may be different we end up
>>>> exposing regions which does not contain any re-distributors.
>>>>
>>>> When booting, Linux will go through all the re-distributor region to
>>>> check whether a property (e.g vPLIs) is available accross all the
>>>> re-distributors. This will result to a data abort on empty regions
>>>> because there are no underlying re-distributor.
>>>>
>>>> So we need to limit the number of regions exposed to the hardware
>>>> domain. The code reworked to only expose the minimun number of regions
>>>> required by the hardware domain. It is assumed the regions will be
>>>> populated starting from the first one.
>>>
>>> I have a question: given that it is possible for a rdist_region to cover
>>> more than 1 cpu, could we get into troubles if the last rdist_region of
>>> the hardware_domain covers 2 cpus, but actually dom0 only has 1 vcpu?
>>> get_vcpu_from_rdist would return NULL and vgic_v3_rdistr_mmio_read/write
>>> would return 0.
>>> This case seems to be handled correctly but I wanted to
>>> double check.
>>
>> 0 means a data abort will be injected into the guest. However, the guest
>> should never touch that because the last valid re-distributor of the regions
>> will have GICR_TYPER.Last set.
>>
>> So the guest OS will stop looking for more re-distributors in that region.
> 
> OK
> 
> 
>>>   >
>>> I think we also need to fix vgic_v3_rdist_count? Today it just returns
>>> vgic_v3_hw.nr_rdist_regions for dom0. It would be bad if we left it
>>> unfixed? If we do that, we might be able to get rid of the modifications
>>> to vgic_v3_real_domain_init.
>>
>> We don't want to fix vgic_v3_rdist_count. The helper returns the maximum
>> re-distributors regions.
> 
> We don't want to or we can't? Because it looks like we would want to fix
> vgic_v3_rdist_count if we could, right? It is called from domain
> specific initialization functions, so theoretically it should return
> domain specific vgic information, not physical information.

We don't want to fix in the current design.

> 
> 
>> This is used to compute the number of IO handlers and
>> allocating the array storing the regions.
>>
>> I am pretty sure you will say we will waste memory. However, at the moment,
>> we need to know the number of IO handlers much before we know the number of
>> vCPUs. For the array, we would need to go through the regions twice (regions
>> may not be the same size so we can't infer easily the number needed). Overall,
>> the amount of memory used is the same as today (so not really a waste per-se).
>>
>> It might be possible to limit that once we reworked the common code to know
>> the number of vCPUs earlier on (see discussion on patch #1).
> 
> Yeah, this is nasty, but it is clear that until we rework the code to
> set d->max_vcpus earlier it won't get fixed. Nothing we can do here.
> 
> So, I think ideally we would want to fix vgic_v3_rdist_count, but today
> we can't. Maybe we could rename vgic_v3_rdist_count to
> vgic_v3_hw_rdist_count, and add a short TODO comment somewhere in the
> file?
> 

Which would be wrong as the function don't always return the number of 
rdist for the HW.

A better name would be vgic_v3_max_rdist_count(struct domain *d).

Cheers,
-- 
Julien Grall

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

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

[Andrew Cooper]

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@arm.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>
>>>>>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4769 bytes --]

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@arm.com>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>>>
> >>>>>>> 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@kernel.org>

:-)

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 2/3] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Stefano Stabellini]

On Fri, 28 Sep 2018, Julien Grall wrote:
> On 09/28/2018 12:34 AM, Stefano Stabellini wrote:
> > On Wed, 26 Sep 2018, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 09/25/2018 09:38 PM, Stefano Stabellini wrote:
> > > > On Tue, 4 Sep 2018, Julien Grall wrote:
> > > > > At the moment, Xen is assuming the hardware domain will have the same
> > > > > number of re-distributor regions as the host. However, as the
> > > > > number of CPUs or the stride (e.g on GICv4) may be different we end up
> > > > > exposing regions which does not contain any re-distributors.
> > > > > 
> > > > > When booting, Linux will go through all the re-distributor region to
> > > > > check whether a property (e.g vPLIs) is available accross all the
> > > > > re-distributors. This will result to a data abort on empty regions
> > > > > because there are no underlying re-distributor.
> > > > > 
> > > > > So we need to limit the number of regions exposed to the hardware
> > > > > domain. The code reworked to only expose the minimun number of regions
> > > > > required by the hardware domain. It is assumed the regions will be
> > > > > populated starting from the first one.
> > > > 
> > > > I have a question: given that it is possible for a rdist_region to cover
> > > > more than 1 cpu, could we get into troubles if the last rdist_region of
> > > > the hardware_domain covers 2 cpus, but actually dom0 only has 1 vcpu?
> > > > get_vcpu_from_rdist would return NULL and vgic_v3_rdistr_mmio_read/write
> > > > would return 0.
> > > > This case seems to be handled correctly but I wanted to
> > > > double check.
> > > 
> > > 0 means a data abort will be injected into the guest. However, the guest
> > > should never touch that because the last valid re-distributor of the
> > > regions
> > > will have GICR_TYPER.Last set.
> > > 
> > > So the guest OS will stop looking for more re-distributors in that region.
> > 
> > OK
> > 
> > 
> > > >   >
> > > > I think we also need to fix vgic_v3_rdist_count? Today it just returns
> > > > vgic_v3_hw.nr_rdist_regions for dom0. It would be bad if we left it
> > > > unfixed? If we do that, we might be able to get rid of the modifications
> > > > to vgic_v3_real_domain_init.
> > > 
> > > We don't want to fix vgic_v3_rdist_count. The helper returns the maximum
> > > re-distributors regions.
> > 
> > We don't want to or we can't? Because it looks like we would want to fix
> > vgic_v3_rdist_count if we could, right? It is called from domain
> > specific initialization functions, so theoretically it should return
> > domain specific vgic information, not physical information.
> 
> We don't want to fix in the current design.
> 
> > 
> > 
> > > This is used to compute the number of IO handlers and
> > > allocating the array storing the regions.
> > > 
> > > I am pretty sure you will say we will waste memory. However, at the
> > > moment,
> > > we need to know the number of IO handlers much before we know the number
> > > of
> > > vCPUs. For the array, we would need to go through the regions twice
> > > (regions
> > > may not be the same size so we can't infer easily the number needed).
> > > Overall,
> > > the amount of memory used is the same as today (so not really a waste
> > > per-se).
> > > 
> > > It might be possible to limit that once we reworked the common code to
> > > know
> > > the number of vCPUs earlier on (see discussion on patch #1).
> > 
> > Yeah, this is nasty, but it is clear that until we rework the code to
> > set d->max_vcpus earlier it won't get fixed. Nothing we can do here.
> > 
> > So, I think ideally we would want to fix vgic_v3_rdist_count, but today
> > we can't. Maybe we could rename vgic_v3_rdist_count to
> > vgic_v3_hw_rdist_count, and add a short TODO comment somewhere in the
> > file?
> > 
> 
> Which would be wrong as the function don't always return the number of rdist
> for the HW.
> 
> A better name would be vgic_v3_max_rdist_count(struct domain *d).

I am OK with that

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

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

[Andrew Cooper]

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@arm.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>>>
>>>>>>>>> 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@kernel.org>

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 :)

~Andrew

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

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

[Julien Grall]

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@arm.com>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>>>>
>>>>>>>>>> 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@kernel.org>
> 
> 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?

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

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

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@arm.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>>>>>
>>>>>>>>>>> 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@kernel.org>
>>
>> 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?

~Andrew

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

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

[Julien Grall]

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@arm.com>
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>>>>>>
>>>>>>>>>>>> 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@kernel.org>
>>>
>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel