Re: [Xen-devel] Xen Dom0 boot failure on platform that supports ARM GICv4

[Julien Grall <julien.grall@xxxxxxx>]

On 09/03/2018 07:37 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@xxxxxxx]
>> Sent: 03 September 2018 18:14
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
>> xen-devel@xxxxxxxxxxxxx
>> Cc: sstabellini@xxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; Andre
>> Przywara <andre.przywara@xxxxxxx>
>> Subject: Re: Xen Dom0 boot failure on platform that supports ARM GICv4
>>
>>
>>
>> On 03/09/18 17:54, Shameerali Kolothum Thodi wrote:
>>> Hi Julien,
>>>
>>> Thanks for taking a look at this.
>>>
>>>> -----Original Message-----
>>>> From: Julien Grall [mailto:julien.grall@xxxxxxx]
>>>> Sent: 03 September 2018 17:13
>>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
>>>> xen-devel@xxxxxxxxxxxxx
>>>> Cc: sstabellini@xxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; Andre
>>>> Przywara <andre.przywara@xxxxxxx>
>>>> Subject: Re: Xen Dom0 boot failure on platform that supports ARM GICv4
>>>>
>>>>
>>>>
>>>> On 03/09/18 15:53, Shameerali Kolothum Thodi wrote:
>>>>> Hi,
>>>>
>>>> Hello,
>>>>
>>>>> I am trying to boot xen(stable-4.11) on one of our ARM64 boards which
>>>>> has support for GICv4.
>>>>>
>>>>> But dom0(kernel 4.18) boot fails with the below trap,
>>>>>
>>>>> XEN) ............done.
>>>>> (XEN) Std. Loglevel: All
>>>>> (XEN) Guest Loglevel: All
>>>>> (XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch
>>>>> input to Xen)
>>>>> (XEN) Freed 304kB init memory.
>>>>> (XEN) traps.c:2007:d0v0 HSR=0x93800004 pc=0xffff00000841af04
>>>>> gva=0xffff00000b10ffe8 gpa=0x004000aa10ffe8
>>>>
>>>> Which bits of Linux is trying to access the region?
>>>
>>> I think it is the gic_iterate_rdists() as the offset just before this is 
>>> ffe8, which
>> is GICR_PIDR2
>>>
>>>>>
>>>>> After a bit of debugging, it looks like, the GICR size used in
>>>> vgic_v3_domain_init()
>>>>> is GICv4 GICR size(256K) and this upsets the first_cpu calculations.
>>>>
>>>> Can you expand what you mean by upset? What's wrong with the first_cpu
>>>> calculations.
>>>
>>> What I meant is, since this is a GICv4, the vgic_v3_hw.regions[i]->size is 
>>> set to
>> 256K and
>>> since first_cpu is calculated like,
>>>
>>>     first_cpu += size /GICV3_GICR_SIZE;
>>>
>>> gets wrong as what I am seeing is,
>>>
>>> (XEN) frst_cpu 2
>>> (XEN) first_cpu 4
>>> (XEN) first_cpu 6
>>> (XEN) first_cpu 8
>>> (XEN) first_cpu 10
>>> (XEN) first_cpu 12
>>> (XEN) first_cpu 14
>>> .....
>>> (XEN) first_cpu 192
>>>
>>> But the original number of CPUS are only 96. Hence I thought this is wrong.
>>
>> This is perfectly fine. Until recently it was not possible to know the
>> number of vCPUs at domain creation. So the function is computing the
>> first CPU for all the regions.
> 
>> With the recent change, it would be possible to only compute what is
>> necessary.
> 
> Ah..alright. This was not clear to me.
>   
>>>>>
>>>>> Since dom0 gicv3 is also an emulated one, I think the size should be
>>>>> restricted to use the GICv3 GICR size(128K). I have made the below
>>>>> changes and is able to boot dom0 now.
>>>>>
>>>>> But not sure, this is the right approach to fix the issue. Please let me
>>>>> know your thoughts.
>>>>>
>>>>> Thanks,
>>>>> Shameer
>>>>>
>>>>> ---->8-------------
>>>>>
>>>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>>>> index b2ed0f8..bf028cc 100644
>>>>> --- a/xen/arch/arm/gic-v3.c
>>>>> +++ b/xen/arch/arm/gic-v3.c
>>>>> @@ -1783,7 +1783,8 @@ static int __init gicv3_init(void)
>>>>>         reg = readl_relaxed(GICD + GICD_TYPER);
>>>>>         intid_bits = GICD_TYPE_ID_BITS(reg);
>>>>>
>>>>> -    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
>>>> intid_bits);
>>>>> +    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
>>>>> +                                intid_bits, gic_dist_supports_dvis());
>>>>>         gicv3_init_v2();
>>>>>
>>>>>         spin_lock_init(&gicv3.lock);
>>>>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>>>>> index 4b42739..0f53d88 100644
>>>>> --- a/xen/arch/arm/vgic-v3.c
>>>>> +++ b/xen/arch/arm/vgic-v3.c
>>>>> @@ -59,18 +59,21 @@ static struct {
>>>>>         unsigned int nr_rdist_regions;
>>>>>         const struct rdist_region *regions;
>>>>>         unsigned int intid_bits;  /* Number of interrupt ID bits */
>>>>> +    bool dvis;
>>>>>     } vgic_v3_hw;
>>>>>
>>>>>     void vgic_v3_setup_hw(paddr_t dbase,
>>>>>                           unsigned int nr_rdist_regions,
>>>>>                           const struct rdist_region *regions,
>>>>> -                      unsigned int intid_bits)
>>>>> +                      unsigned int intid_bits,
>>>>> +                      bool dvis)
>>>>>     {
>>>>>         vgic_v3_hw.enabled = true;
>>>>>         vgic_v3_hw.dbase = dbase;
>>>>>         vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
>>>>>         vgic_v3_hw.regions = regions;
>>>>>         vgic_v3_hw.intid_bits = intid_bits;
>>>>> +    vgic_v3_hw.dvis = dvis;
>>>>>     }
>>>>>
>>>>>     static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain *d, uint64_t
>>>> irouter)
>>>>> @@ -1673,6 +1676,9 @@ static int vgic_v3_domain_init(struct domain *d)
>>>>>             {
>>>>>                 paddr_t size = vgic_v3_hw.regions[i].size;
>>>>>
>>>>> +            if (vgic_v3_hw.dvis && (size == GICV4_GICR_SIZE))
>>>>> +                 size = GICV3_GICR_SIZE;
>>>>
>>>> vgic_v3_hw.regions is describing the regions in the layout that could
>>>> hold re-distributor. You can have multiple re-distributor per region.
>>>>
>>>> The variable size holds the size of the region, not the size of the
>>>> re-distributor.
>>>>
>>>> I am not sure to understand why you want to restrict the size of the
>>>> region here because GICV4_GICR_SIZE is a multiple of GICV3_GICR_SIZE. So
>>>> you should be able to fit 2 re-distributors per region.
>>>>
>>>> It looks like to me the re-distributor regions are not reported
>>>> correctly or Dom0 thinks it is on GICv4. Can you provide a bit more
>>>> details on the function that cause the crash and some logs from Linux?
>>>
>>> Ok. I added few prints along the vgic mmio read path and this is what
>> happens
>>> before the trap.
>>>
>>>       vgic_v3_rdistr_mmio_read()
>>>            get_vcpu_from_rdist()  -->returns NULL here for 0x004000aa10ffe8
>> which
>>>                                                       actually belongs to 
>>> cpu id 48 as per the log
>> below
>>
>> Do you mean region id 48? So if I get it correctly, you are trying to
>> access re-distributor for vCPU ID 96.
> 
> Hmm..I was under the impression that there is a one to one map here.
> And you are right, it is indeed vcpu id 96 which is invalid.
>> [...]
>>
> 
>>> If I remember correctly there was no logs from Dom0, but I need to double
>>> check the Dom0 cmdline option to see earlycon was set.
>>>
>>> I could also enable/add any prints that you think will help and rerun. 
>>> Please
>>> let me know
>>
>> I may have an idea what is happening. As we populate more regions than
>> necessary, it is possible that Linux is trying to access them. Would it
>> be possible to add some debug in the Linux function gic_iterate_rdists
>> to know what the kernel is trying to read?
> 
> Ok, enabled earlycon for Dom0. Please find the log below,

Thank you for the log. I now have an idea what's is going wrong. The function
gic_iterate_rdists can be used to go through all the re-distributor (for 
instance
to check whether vLPIs is available).

Because some of the regions are empty (i.e not emulated), you end up to trap. 
Your
patch solves the problem by making regions not empty in the case of GICv4. But I
think this can also happen when the number of vCPUs for Dom0 get restricted.

Can you have a try at the patch below? I haven't tested on ACPI.

If that works for you, I will add the DT case, clean it up and send it.

Cheers,

>From c1fe63fae976c9d4bf17551d141748c04febab37 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@xxxxxxx>
Date: Tue, 4 Sep 2018 12:10:39 +0100
Subject: [PATCH] xen/arm: gic-v3: Don't create empty re-distributor regions

Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
---
 xen/arch/arm/gic-v3.c  |   2 +-
 xen/arch/arm/vgic-v3.c | 159 ++++++++++++++++++++++++++++---------------------
 2 files changed, 92 insertions(+), 69 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b2ed0f8b55..eef6776064 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1503,7 +1503,7 @@ 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++ )
+    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 4b42739a52..06a9972421 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1573,9 +1573,91 @@ static const struct mmio_handler_ops 
vgic_distr_mmio_handler = {
     .write = vgic_v3_distr_mmio_write,
 };
 
+static inline unsigned int vgic_v3_rdist_count(struct domain *d)
+{
+    /*
+     * Normally there is only one GICv3 redistributor region.
+     * The GICv3 DT binding provisions for multiple regions, since there are
+     * platforms out there which need those (multi-socket systems).
+     * For Dom0 we have to live with the MMIO layout the hardware provides,
+     * so we have to copy the multiple regions - as the first region may not
+     * provide enough space to hold all redistributors we need.
+     * However DomU get a constructed memory map, so we can go with
+     * the architected single redistributor region.
+     */
+    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
+               GUEST_GICV3_RDIST_REGIONS;
+}
+
+static int vgic_v3_initialize_rdists(struct domain *d)
+{
+    struct vgic_rdist_region *rdist_regions;
+    int rdist_count, i;
+
+   /* Allocate memory for Re-distributor regions */
+    rdist_count = vgic_v3_rdist_count(d);
+
+    rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
+    if ( !rdist_regions )
+        return -ENOMEM;
+
+    d->arch.vgic.nr_regions = rdist_count;
+    d->arch.vgic.rdist_regions = rdist_regions;
+
+    if ( is_hardware_domain(d) )
+    {
+        unsigned int first_cpu = 0;
+
+        for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ )
+        {
+            paddr_t size = vgic_v3_hw.regions[i].size;
+
+            d->arch.vgic.rdist_regions[i].base = vgic_v3_hw.regions[i].base;
+            d->arch.vgic.rdist_regions[i].size = size;
+
+            /* Set the first CPU handled by this region */
+            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
+
+            first_cpu += size / GICV3_GICR_SIZE;
+
+            if ( first_cpu >= d->max_vcpus )
+                break;
+        }
+
+        /* Update with the actual number of regions used */
+        d->arch.vgic.nr_regions = i + 1;
+    }
+    else
+    {
+        /* A single Re-distributor region is mapped for the guest. */
+        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
+
+        /* The first redistributor should contain enough space for all CPUs */
+        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GICV3_GICR_SIZE) < 
MAX_VIRT_CPUS);
+        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
+        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
+        d->arch.vgic.rdist_regions[0].first_cpu = 0;
+    }
+
+    /*
+     * Register mmio handler per contiguous region occupied by the
+     * redistributors. The handler will take care to choose which
+     * redistributor is targeted.
+     */
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
+    {
+        struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];
+
+        register_mmio_handler(d, &vgic_rdistr_mmio_handler,
+                              region->base, region->size, region);
+    }
+
+    return 0;
+}
+
 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;
@@ -1583,6 +1665,13 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     /* Convenient alias */
     struct domain *d = v->domain;
 
+    if ( v->vcpu_id == 0 )
+    {
+        rc = vgic_v3_initialize_rdists(v->domain);
+        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.
@@ -1625,36 +1714,9 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     return 0;
 }
 
-static inline unsigned int vgic_v3_rdist_count(struct domain *d)
-{
-    /*
-     * Normally there is only one GICv3 redistributor region.
-     * The GICv3 DT binding provisions for multiple regions, since there are
-     * platforms out there which need those (multi-socket systems).
-     * For Dom0 we have to live with the MMIO layout the hardware provides,
-     * so we have to copy the multiple regions - as the first region may not
-     * provide enough space to hold all redistributors we need.
-     * However DomU get a constructed memory map, so we can go with
-     * the architected single redistributor region.
-     */
-    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
-               GUEST_GICV3_RDIST_REGIONS;
-}
-
 static int vgic_v3_domain_init(struct domain *d)
 {
-    struct vgic_rdist_region *rdist_regions;
-    int rdist_count, i, ret;
-
-    /* Allocate memory for Re-distributor regions */
-    rdist_count = vgic_v3_rdist_count(d);
-
-    rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
-    if ( !rdist_regions )
-        return -ENOMEM;
-
-    d->arch.vgic.nr_regions = rdist_count;
-    d->arch.vgic.rdist_regions = rdist_regions;
+    int ret;
 
     rwlock_init(&d->arch.vgic.pend_lpi_tree_lock);
     radix_tree_init(&d->arch.vgic.pend_lpi_tree);
@@ -1665,38 +1727,12 @@ static int vgic_v3_domain_init(struct domain *d)
      */
     if ( is_hardware_domain(d) )
     {
-        unsigned int first_cpu = 0;
-
         d->arch.vgic.dbase = vgic_v3_hw.dbase;
-
-        for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ )
-        {
-            paddr_t size = vgic_v3_hw.regions[i].size;
-
-            d->arch.vgic.rdist_regions[i].base = vgic_v3_hw.regions[i].base;
-            d->arch.vgic.rdist_regions[i].size = size;
-
-            /* Set the first CPU handled by this region */
-            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
-
-            first_cpu += size / GICV3_GICR_SIZE;
-        }
-
         d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
     }
     else
     {
         d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
-
-        /* A single Re-distributor region is mapped for the guest. */
-        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
-
-        /* The first redistributor should contain enough space for all CPUs */
-        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GICV3_GICR_SIZE) < 
MAX_VIRT_CPUS);
-        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
-        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
-        d->arch.vgic.rdist_regions[0].first_cpu = 0;
-
         /*
          * TODO: only SPIs for now, adjust this when guests need LPIs.
          * Please note that this value just describes the bits required
@@ -1715,19 +1751,6 @@ static int vgic_v3_domain_init(struct domain *d)
     register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
                           SZ_64K, NULL);
 
-    /*
-     * Register mmio handler per contiguous region occupied by the
-     * redistributors. The handler will take care to choose which
-     * redistributor is targeted.
-     */
-    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
-    {
-        struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];
-
-        register_mmio_handler(d, &vgic_rdistr_mmio_handler,
-                              region->base, region->size, region);
-    }
-
     d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
 
     return 0;

-- 
Julien Grall

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