[PATCH v1 0/6] Unflattening and relocation of host device tree

[PATCH v1 0/6] Unflattening and relocation of host device tree

[Oleksii Kurochko]

The current patch series introduces the relocation of the host device tree file
to free up low memory and also it is expected that discard_initial_modules()
will be called sooner or later, it will discard the FDT boot module,
and remove_early_mappings() will destroy the early mappings.

In addition to relocation, unflattening is introduced to create the tree of
struct device_node for the host device tree.

To implement this, several things have been introduced:
 - destroy_xen_mappings() function, which removes page mappings from Xen's
   page tables. This is necessary for clear_fixmap().
 - {set,clear}_fixmap() functions to manage mappings in the fixmap region,
   which are expected to be used in copy_from_paddr() to copy the FDT to Xen's
   heap.
 - A new config HAS_CMO is introduced (in anticipation of future use). This is
   despite the fact that hardware ( "available" to me ) with the hypervisor
   extension is generally I/O-coherent ( and it is preferred way mentioned in
   the RISC-V spec ) and should not be an issue in QEMU as it doesn't emulate
   caches.
   This config introduces stubs for clean_and_invalidate_dcache_va_range()
   and clean_dcache_va_range(), which are expected to be used in
   copy_from_paddr() and flush_page_to_ram(), which in turn are expected to be
   used during the call to xmalloc_bytes() in relocate_fdt().
 - The introduction of copy_from_paddr() to copy the FDT to an address
   allocated in Xen's heap.

Oleksii Kurochko (6):
  xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page
    tables
  xen/riscv: reorder includes in asm/page.h alphabetically
  xen/riscv: add {set,clear}_fixmap() functions for managing fixmap
    entries
  xen/riscv: introduce cache management operations (CMO)
  xen/riscv: implement relocate_fdt()
  xen/riscv: relocating and unflattening host device tree

 xen/arch/riscv/Kconfig              |  3 ++
 xen/arch/riscv/include/asm/fixmap.h |  5 +++
 xen/arch/riscv/include/asm/mm.h     | 10 ++++-
 xen/arch/riscv/include/asm/page.h   | 30 +++++++++++++--
 xen/arch/riscv/mm.c                 |  9 ++---
 xen/arch/riscv/pt.c                 | 27 ++++++++++++++
 xen/arch/riscv/setup.c              | 57 ++++++++++++++++++++++++++++-
 7 files changed, 128 insertions(+), 13 deletions(-)

-- 
2.47.0

[PATCH v1 1/6] xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page tables

[Oleksii Kurochko]

Introduce the destroy_xen_mappings() function, which removes page
mappings in Xen's page tables between a start address s and an end
address e.
The function ensures that both s and e are page-aligned
and verifies that the start address is less than or equal to the end
address before calling pt_update() to invalidate the mappings.
The pt_update() function is called with INVALID_MFN and PTE_VALID=0
in the flags, which tell pt_update() to remove mapping. No additional
ASSERT() is required to check these arguments, as they are hardcoded in
the call to pt_update() within destroy_xen_mappings().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/mm.c | 6 ------
 xen/arch/riscv/pt.c | 8 ++++++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 9359dc7f33..f2bf279bac 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -360,12 +360,6 @@ int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
     return 0;
 }
 
-int destroy_xen_mappings(unsigned long s, unsigned long e)
-{
-    BUG_ON("unimplemented");
-    return -1;
-}
-
 void share_xen_page_with_guest(struct page_info *page, struct domain *d,
                                enum XENSHARE_flags flags)
 {
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index d62aceb36c..8d35ef5ca8 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
     return pt_update(virt, mfn, nr_mfns, flags);
 }
 
+int destroy_xen_mappings(unsigned long s, unsigned long e)
+{
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
+    ASSERT(s <= e);
+    return pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0);
+}
+
 int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 {
     return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);
-- 
2.47.0

Re: [PATCH v1 1/6] xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page tables

[Jan Beulich]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> Introduce the destroy_xen_mappings() function, which removes page
> mappings in Xen's page tables between a start address s and an end
> address e.
> The function ensures that both s and e are page-aligned
> and verifies that the start address is less than or equal to the end
> address before calling pt_update() to invalidate the mappings.
> The pt_update() function is called with INVALID_MFN and PTE_VALID=0
> in the flags, which tell pt_update() to remove mapping. No additional
> ASSERT() is required to check these arguments, as they are hardcoded in
> the call to pt_update() within destroy_xen_mappings().
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

However, ...

> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
>      return pt_update(virt, mfn, nr_mfns, flags);
>  }
>  
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> +{
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> +    ASSERT(s <= e);
> +    return pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0);
> +}

... I'm unconvinced the constraints need to be this strict. You could,
for example, very well just avoiding to call pt_update() when s > e
(or really when s >= e). Thoughts?

Jan

Re: [PATCH v1 1/6] xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page tables

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]


On 12/9/24 3:23 PM, Jan Beulich wrote:
> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>> Introduce the destroy_xen_mappings() function, which removes page
>> mappings in Xen's page tables between a start address s and an end
>> address e.
>> The function ensures that both s and e are page-aligned
>> and verifies that the start address is less than or equal to the end
>> address before calling pt_update() to invalidate the mappings.
>> The pt_update() function is called with INVALID_MFN and PTE_VALID=0
>> in the flags, which tell pt_update() to remove mapping. No additional
>> ASSERT() is required to check these arguments, as they are hardcoded in
>> the call to pt_update() within destroy_xen_mappings().
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich<jbeulich@suse.com>
>
> However, ...
>
>> --- a/xen/arch/riscv/pt.c
>> +++ b/xen/arch/riscv/pt.c
>> @@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
>>       return pt_update(virt, mfn, nr_mfns, flags);
>>   }
>>   
>> +int destroy_xen_mappings(unsigned long s, unsigned long e)
>> +{
>> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>> +    ASSERT(s <= e);
>> +    return pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0);
>> +}
> ... I'm unconvinced the constraints need to be this strict. You could,
> for example, very well just avoiding to call pt_update() when s > e
> (or really when s >= e). Thoughts?

On one hand, we could simply avoid calling |pt_update()|, but on the 
other hand, this approach might cause us to miss a bug without any 
notification.

Given that this is an|ASSERT()| that only triggers in debug builds and is unlikely to occur,
I believe it is not critical to include the|ASSERT()| here. Additionally, avoiding an extra
|if| condition helps prevent any potential performance impact. However, the|if| condition
would likely evaluate to true most of the time, allowing hardware optimizations to handle
it efficiently.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 2892 bytes --]

Re: [PATCH v1 1/6] xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page tables

[Jan Beulich]

On 10.12.2024 12:14, Oleksii Kurochko wrote:
> 
> On 12/9/24 3:23 PM, Jan Beulich wrote:
>> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>>> Introduce the destroy_xen_mappings() function, which removes page
>>> mappings in Xen's page tables between a start address s and an end
>>> address e.
>>> The function ensures that both s and e are page-aligned
>>> and verifies that the start address is less than or equal to the end
>>> address before calling pt_update() to invalidate the mappings.
>>> The pt_update() function is called with INVALID_MFN and PTE_VALID=0
>>> in the flags, which tell pt_update() to remove mapping. No additional
>>> ASSERT() is required to check these arguments, as they are hardcoded in
>>> the call to pt_update() within destroy_xen_mappings().
>>>
>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> Acked-by: Jan Beulich<jbeulich@suse.com>
>>
>> However, ...
>>
>>> --- a/xen/arch/riscv/pt.c
>>> +++ b/xen/arch/riscv/pt.c
>>> @@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
>>>       return pt_update(virt, mfn, nr_mfns, flags);
>>>   }
>>>   +int destroy_xen_mappings(unsigned long s, unsigned long e)
>>> +{
>>> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>>> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>>> +    ASSERT(s <= e);
>>> +    return pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0);
>>> +}
>> ... I'm unconvinced the constraints need to be this strict. You could,
>> for example, very well just avoiding to call pt_update() when s > e
>> (or really when s >= e). Thoughts?
> 
> On one hand, we could simply avoid calling |pt_update()|, but on the other hand, this approach might cause us to miss a bug without any notification.
> 
> Given that this is an|ASSERT()| that only triggers in debug builds and is unlikely to occur,
> I believe it is not critical to include the|ASSERT()| here.

Right, and that is one of the points. In release builds a potential
bad call here wouldn't be prevented if there's just an assertion.
Unlike if there was an if() instead (perhaps with ASSERT_UNREACHABLE()
on its "else" path).

> Additionally, avoiding an extra
> |if| condition helps prevent any potential performance impact. However, the|if| condition
> would likely evaluate to true most of the time, allowing hardware optimizations to handle
> it efficiently.

I don't think we need to be afraid of performance issues here.

Jan

[PATCH v1 2/6] xen/riscv: reorder includes in asm/page.h alphabetically

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 91b1194b55..bf3f75e85d 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -5,8 +5,8 @@
 
 #ifndef __ASSEMBLY__
 
-#include <xen/const.h>
 #include <xen/bug.h>
+#include <xen/const.h>
 #include <xen/types.h>
 
 #include <asm/atomic.h>
-- 
2.47.0

Re: [PATCH v1 2/6] xen/riscv: reorder includes in asm/page.h alphabetically

[Jan Beulich]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH v1 3/6] xen/riscv: add {set,clear}_fixmap() functions for managing fixmap entries

[Oleksii Kurochko]

Introduce set_fixmap() and clear_fixmap() functions to manage mappings
in the fixmap region. The set_fixmap() function maps a 4k page ( as only L0
is expected to be updated; look at setup_fixmap_mappings() ) at a specified
fixmap entry using map_pages_to_xen(), while clear_fixmap() removes the
mapping from a fixmap entry by calling destroy_xen_mappings().

Both functions ensure that the operations succeed by asserting that their
respective calls (map_pages_to_xen() and destroy_xen_mappings()) return 0.
A `BUG_ON` check is used to trigger a failure if any issues occur during
the mapping or unmapping process.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/fixmap.h |  5 +++++
 xen/arch/riscv/pt.c                 | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
index 818c8ce07b..e399a15f53 100644
--- a/xen/arch/riscv/include/asm/fixmap.h
+++ b/xen/arch/riscv/include/asm/fixmap.h
@@ -32,6 +32,11 @@
  */
 extern pte_t xen_fixmap[];
 
+/* Map a page in a fixmap entry */
+void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags);
+/* Remove a mapping from a fixmap entry */
+void clear_fixmap(unsigned int map);
+
 #define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
 
 static inline unsigned int virt_to_fix(vaddr_t vaddr)
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index 8d35ef5ca8..ed9a943d4c 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -8,6 +8,7 @@
 #include <xen/pmap.h>
 #include <xen/spinlock.h>
 
+#include <asm/fixmap.h>
 #include <asm/flushtlb.h>
 #include <asm/page.h>
 
@@ -433,3 +434,21 @@ int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 {
     return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);
 }
+
+/* Map a 4k page in a fixmap entry */
+void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags)
+{
+    int res;
+
+    res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL);
+    BUG_ON(res != 0);
+}
+
+/* Remove a mapping from a fixmap entry */
+void clear_fixmap(unsigned int map)
+{
+    int res;
+
+    res = destroy_xen_mappings(FIXMAP_ADDR(map), FIXMAP_ADDR(map) + PAGE_SIZE);
+    BUG_ON(res != 0);
+}
-- 
2.47.0

Re: [PATCH v1 3/6] xen/riscv: add {set,clear}_fixmap() functions for managing fixmap entries

[Jan Beulich]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> Introduce set_fixmap() and clear_fixmap() functions to manage mappings
> in the fixmap region. The set_fixmap() function maps a 4k page ( as only L0
> is expected to be updated; look at setup_fixmap_mappings() ) at a specified
> fixmap entry using map_pages_to_xen(), while clear_fixmap() removes the
> mapping from a fixmap entry by calling destroy_xen_mappings().
> 
> Both functions ensure that the operations succeed by asserting that their
> respective calls (map_pages_to_xen() and destroy_xen_mappings()) return 0.
> A `BUG_ON` check is used to trigger a failure if any issues occur during
> the mapping or unmapping process.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

However, ...

> @@ -433,3 +434,21 @@ int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>  {
>      return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);
>  }
> +
> +/* Map a 4k page in a fixmap entry */
> +void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags)
> +{
> +    int res;
> +
> +    res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL);
> +    BUG_ON(res != 0);
> +}

... imo in such cases it is preferable to go without a local variable:

    if ( map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL) != 0 )
        BUG();

Just to double check: Iirc this BUG would in particular trigger when trying
to set a fixmap slot that was already set, and not intermediately cleared?

Jan

Re: [PATCH v1 3/6] xen/riscv: add {set,clear}_fixmap() functions for managing fixmap entries

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]


On 12/9/24 3:29 PM, Jan Beulich wrote:
> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>> Introduce set_fixmap() and clear_fixmap() functions to manage mappings
>> in the fixmap region. The set_fixmap() function maps a 4k page ( as only L0
>> is expected to be updated; look at setup_fixmap_mappings() ) at a specified
>> fixmap entry using map_pages_to_xen(), while clear_fixmap() removes the
>> mapping from a fixmap entry by calling destroy_xen_mappings().
>>
>> Both functions ensure that the operations succeed by asserting that their
>> respective calls (map_pages_to_xen() and destroy_xen_mappings()) return 0.
>> A `BUG_ON` check is used to trigger a failure if any issues occur during
>> the mapping or unmapping process.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich<jbeulich@suse.com>
>
> However, ...
>
>> @@ -433,3 +434,21 @@ int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>>   {
>>       return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);
>>   }
>> +
>> +/* Map a 4k page in a fixmap entry */
>> +void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags)
>> +{
>> +    int res;
>> +
>> +    res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL);
>> +    BUG_ON(res != 0);
>> +}
> ... imo in such cases it is preferable to go without a local variable:
>
>      if ( map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL) != 0 )
>          BUG();

I will update that in the next patch version.

>
> Just to double check: Iirc this BUG would in particular trigger when trying
> to set a fixmap slot that was already set, and not intermediately cleared?

Yes, correct.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 2622 bytes --]

[PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)

[Oleksii Kurochko]

KConfig HAS_CMO is introduced to handle if the platform has CMO related
extenstions ( such as Zicbom, Zicboz, Zicbop etc ) or not.

if HAS_CMO isn't set stubs for clean_and_invalidate_dcache_va_range()
and clean_dcache_va_range() are implemented as just returning
-EOPNOTSUPP.

Our current platform is QEMU which doesn't model caches so it should be
fine to follow implementations when HAS_CMO isn't set.

invalidate_icache() is implemented using fence.i instruction as
mentioned in the unpriv spec:
  The FENCE.I instruction was designed to support a wide variety of
  implementations. A simple implementation can flush the local instruction
  cache and the instruction pipeline when the FENCE.I is executed.
  A more complex implementation might snoop the instruction (data) cache
  on every data (instruction) cache miss, or use an inclusive unified
  private L2 cache to invalidate lines from the primary instruction cache
  when they are being written by a local store instruction.
  If instruction and data caches are kept coherent in this way, or if the
  memory system consists of only uncached RAMs, then just the fetch pipeline
  needs to be flushed at a FENCE.I.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Kconfig            |  3 +++
 xen/arch/riscv/include/asm/page.h | 18 +++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 1858004676..4f1fcfd21a 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -14,6 +14,9 @@ config ARCH_DEFCONFIG
 	string
 	default "arch/riscv/configs/tiny64_defconfig"
 
+config HAS_CMO # Cache Management Operations
+	bool
+
 menu "Architecture Features"
 
 source "arch/Kconfig"
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index bf3f75e85d..0f297141d3 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -7,6 +7,7 @@
 
 #include <xen/bug.h>
 #include <xen/const.h>
+#include <xen/errno.h>
 #include <xen/types.h>
 
 #include <asm/atomic.h>
@@ -148,9 +149,24 @@ static inline bool pte_is_mapping(pte_t p)
     return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
 }
 
+#ifndef HAS_CMO
+static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
+{
+    return -EOPNOTSUPP;
+}
+
+static inline int clean_dcache_va_range(const void *p, unsigned long size)
+{
+    return -EOPNOTSUPP;
+}
+#else
+int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size);
+int clean_dcache_va_range(const void *p, unsigned long size);
+#endif
+
 static inline void invalidate_icache(void)
 {
-    BUG_ON("unimplemented");
+    asm volatile ( "fence.i" ::: "memory" );
 }
 
 #define clear_page(page) memset((void *)(page), 0, PAGE_SIZE)
-- 
2.47.0

Re: [PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)

[Jan Beulich]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -14,6 +14,9 @@ config ARCH_DEFCONFIG
>  	string
>  	default "arch/riscv/configs/tiny64_defconfig"
>  
> +config HAS_CMO # Cache Management Operations
> +	bool

Hmm, and nothing ever sets this, and hence ...

> @@ -148,9 +149,24 @@ static inline bool pte_is_mapping(pte_t p)
>      return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
>  }
>  
> +#ifndef HAS_CMO
> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> +{
> +    return -EOPNOTSUPP;
> +}
> +
> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#else
> +int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size);
> +int clean_dcache_va_range(const void *p, unsigned long size);
> +#endif

... all you really provide are stubs and declarations, but no
definition anywhere?

Plus of course this gets us into feature detection territory again: If
RISC-V provided a way to detect presence / absence of certain extensions,
this really shouldn't be a compile time setting, but be determined
dynamically.

>  static inline void invalidate_icache(void)
>  {
> -    BUG_ON("unimplemented");
> +    asm volatile ( "fence.i" ::: "memory" );
>  }

That's a separate extension, Zifencei, which I don't think you can just
assume to be present?

Jan

Re: [PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]


On 12/9/24 3:38 PM, Jan Beulich wrote:
> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/Kconfig
>> +++ b/xen/arch/riscv/Kconfig
>> @@ -14,6 +14,9 @@ config ARCH_DEFCONFIG
>>   	string
>>   	default "arch/riscv/configs/tiny64_defconfig"
>>   
>> +config HAS_CMO # Cache Management Operations
>> +	bool
> Hmm, and nothing ever sets this, and hence ...
>
>> @@ -148,9 +149,24 @@ static inline bool pte_is_mapping(pte_t p)
>>       return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
>>   }
>>   
>> +#ifndef HAS_CMO
>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +#else
>> +int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size);
>> +int clean_dcache_va_range(const void *p, unsigned long size);
>> +#endif
> ... all you really provide are stubs and declarations, but no
> definition anywhere?

Yes, this was done intentionally because:
- I don't have hardware with the CMO extension, so I can't test it. ( QEMU doesn't model cache and so
   there is no need for CMO extension emulation IIUC )
- The instructions used for these functions may be hardware-specific and exist only for particular devices.

It seems useful to have something similar to Linux:
https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135 <https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135>
(There are also custom instructions for THEAD above this macro.)

We could use|ALT_CMO_OP(...)| inside|clean_and_invalidate_dcache_va_range()| and|clean_dcache_va_range()|.
However, I think it would be better to introduce or implement these functions when|HAS_CMO| is set to|y| someday.

As an alternative, we could implement these functions as|panic("need to be implemented\n")| in case when HAS_CMO=y.

Another option is to drop|HAS_CMO| entirely for now and keep the current implementation (|return -EOPNOTSUPP|).
However, with this approach, there's a risk of encountering hard-to-debug issues on platforms with the CMO extension.
And necessity of implementation of these could be missed because there is no any notification...

>
> Plus of course this gets us into feature detection territory again: If
> RISC-V provided a way to detect presence / absence of certain extensions,
> this really shouldn't be a compile time setting, but be determined
> dynamically.

This is the next patch I plan to send after this patch series:
https://gitlab.com/xen-project/people/olkur/xen/-/commit/f81ae67c42854073da5403210c9e31de6b0ee5bd <https://gitlab.com/xen-project/people/olkur/xen/-/commit/f81ae67c42854073da5403210c9e31de6b0ee5bd>

It "detects" available extensions based on a device tree property. While this is not the best approach
(the ideal solution would be hardware having a register that lists all available extensions), it seems to be
the best option available at the moment.

Another option I considered was introducing a new SBI call, delegating the responsibility to OpenSBI
to provide this information.

>
>>   static inline void invalidate_icache(void)
>>   {
>> -    BUG_ON("unimplemented");
>> +    asm volatile ( "fence.i" ::: "memory" );
>>   }
> That's a separate extension, Zifencei, which I don't think you can just
> assume to be present?

Based on the specification:
```
Chapter 34. RV32/64G Instruction Set Listings
One goal of the RISC-V project is that it be used as a stable software development target. For this
purpose, we define a combination of a base ISA (RV32I or RV64I) plus selected standard extensions
(IMAFD, Zicsr, Zifencei) as a "general-purpose" ISA, and we use the abbreviation G for the
IMAFDZicsr_Zifencei combination of instruction-set extensions. This chapter presents opcode maps
and instruction-set listings for RV32G and RV64G
```
and that G is needed to boot Linux kernel ( and so Xen ) I make an assumption that Zifencei will be always
present.

And based on Linux code (https://elixir.bootlin.com/linux/v6.12.4/source/arch/riscv/kernel/cpufeature.c#L676 )
when 'i' is present in riscv,isa property zifencei is present unconditionally.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 6066 bytes --]

Re: [PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)

[Jan Beulich]

On 10.12.2024 13:19, Oleksii Kurochko wrote:
> 
> On 12/9/24 3:38 PM, Jan Beulich wrote:
>> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/Kconfig
>>> +++ b/xen/arch/riscv/Kconfig
>>> @@ -14,6 +14,9 @@ config ARCH_DEFCONFIG
>>>       string
>>>       default "arch/riscv/configs/tiny64_defconfig"
>>>   +config HAS_CMO # Cache Management Operations
>>> +    bool
>> Hmm, and nothing ever sets this, and hence ...
>>
>>> @@ -148,9 +149,24 @@ static inline bool pte_is_mapping(pte_t p)
>>>       return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
>>>   }
>>>   +#ifndef HAS_CMO
>>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +#else
>>> +int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size);
>>> +int clean_dcache_va_range(const void *p, unsigned long size);
>>> +#endif
>> ... all you really provide are stubs and declarations, but no
>> definition anywhere?
> 
> Yes, this was done intentionally because:
> - I don't have hardware with the CMO extension, so I can't test it. ( QEMU doesn't model cache and so
>   there is no need for CMO extension emulation IIUC )
> - The instructions used for these functions may be hardware-specific and exist only for particular devices.
> 
> It seems useful to have something similar to Linux:
> https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135 <https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135>
> (There are also custom instructions for THEAD above this macro.)
> 
> We could use|ALT_CMO_OP(...)| inside|clean_and_invalidate_dcache_va_range()| and|clean_dcache_va_range()|.
> However, I think it would be better to introduce or implement these functions when|HAS_CMO| is set to|y| someday.
> 
> As an alternative, we could implement these functions as|panic("need to be implemented\n")| in case when HAS_CMO=y.

I think this would be well in line with various other stubs you have.

> Another option is to drop|HAS_CMO| entirely for now and keep the current implementation (|return -EOPNOTSUPP|).
> However, with this approach, there's a risk of encountering hard-to-debug issues on platforms with the CMO extension.
> And necessity of implementation of these could be missed because there is no any notification...

Well, callers ought to check return values?

>>>   static inline void invalidate_icache(void)
>>>   {
>>> -    BUG_ON("unimplemented");
>>> +    asm volatile ( "fence.i" ::: "memory" );
>>>   }
>> That's a separate extension, Zifencei, which I don't think you can just
>> assume to be present?
> 
> Based on the specification:
> ```
> Chapter 34. RV32/64G Instruction Set Listings
> One goal of the RISC-V project is that it be used as a stable software development target. For this
> purpose, we define a combination of a base ISA (RV32I or RV64I) plus selected standard extensions
> (IMAFD, Zicsr, Zifencei) as a "general-purpose" ISA, and we use the abbreviation G for the
> IMAFDZicsr_Zifencei combination of instruction-set extensions. This chapter presents opcode maps
> and instruction-set listings for RV32G and RV64G
> ```

Hmm, indeed. That's well hidden in a place I didn't expect it to live at.
Maybe worth a sentence in the description?

> and that G is needed to boot Linux kernel ( and so Xen ) I make an assumption that Zifencei will be always
> present.

I'd be a little careful here. Xen may be used in Linux-free environments.
I notice arch.mk specifies rv64g, yet I'm uncertain we shouldn't relax
that at some point.

> And based on Linux code (https://elixir.bootlin.com/linux/v6.12.4/source/arch/riscv/kernel/cpufeature.c#L676 )
> when 'i' is present in riscv,isa property zifencei is present unconditionally.

That looks questionable to me. I don't think Zifencei can be inferred from
I. Historically it was, and imo that's what the comment there says. Plus
it is dependent upon acpi_disabled.

Jan

Re: [PATCH v1 4/6] xen/riscv: introduce cache management operations (CMO)

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 5026 bytes --]


On 12/10/24 1:39 PM, Jan Beulich wrote:
> On 10.12.2024 13:19, Oleksii Kurochko wrote:
>> On 12/9/24 3:38 PM, Jan Beulich wrote:
>>> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>>>> --- a/xen/arch/riscv/Kconfig
>>>> +++ b/xen/arch/riscv/Kconfig
>>>> @@ -14,6 +14,9 @@ config ARCH_DEFCONFIG
>>>>        string
>>>>        default "arch/riscv/configs/tiny64_defconfig"
>>>>    +config HAS_CMO # Cache Management Operations
>>>> +    bool
>>> Hmm, and nothing ever sets this, and hence ...
>>>
>>>> @@ -148,9 +149,24 @@ static inline bool pte_is_mapping(pte_t p)
>>>>        return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
>>>>    }
>>>>    +#ifndef HAS_CMO
>>>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
>>>> +{
>>>> +    return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
>>>> +{
>>>> +    return -EOPNOTSUPP;
>>>> +}
>>>> +#else
>>>> +int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size);
>>>> +int clean_dcache_va_range(const void *p, unsigned long size);
>>>> +#endif
>>> ... all you really provide are stubs and declarations, but no
>>> definition anywhere?
>> Yes, this was done intentionally because:
>> - I don't have hardware with the CMO extension, so I can't test it. ( QEMU doesn't model cache and so
>>    there is no need for CMO extension emulation IIUC )
>> - The instructions used for these functions may be hardware-specific and exist only for particular devices.
>>
>> It seems useful to have something similar to Linux:
>> https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135 <https://elixir.bootlin.com/linux/v6.6.64/source/arch/riscv/include/asm/errata_list.h#L135>
>> (There are also custom instructions for THEAD above this macro.)
>>
>> We could use|ALT_CMO_OP(...)| inside|clean_and_invalidate_dcache_va_range()| and|clean_dcache_va_range()|.
>> However, I think it would be better to introduce or implement these functions when|HAS_CMO| is set to|y| someday.
>>
>> As an alternative, we could implement these functions as|panic("need to be implemented\n")| in case when HAS_CMO=y.
> I think this would be well in line with various other stubs you have.
>
>> Another option is to drop|HAS_CMO| entirely for now and keep the current implementation (|return -EOPNOTSUPP|).
>> However, with this approach, there's a risk of encountering hard-to-debug issues on platforms with the CMO extension.
>> And necessity of implementation of these could be missed because there is no any notification...
> Well, callers ought to check return values?

Yeah, callers should check return value but we still have to introduce then a new KConfig ( config QEMU ) and then implementation
will look like:
   static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
   {
   #ifdef CONFIG_QEMU
     return 0;
   #else
     return -EOPNOTSUPP;
   #endif
   }

   static inline int clean_dcache_va_range(const void *p, unsigned long size)
   {
   #ifdef CONFIG_QEMU
     return 0;
   #else
     return -EOPNOTSUPP;
   #endif
   }

>
>>>>    static inline void invalidate_icache(void)
>>>>    {
>>>> -    BUG_ON("unimplemented");
>>>> +    asm volatile ( "fence.i" ::: "memory" );
>>>>    }
>>> That's a separate extension, Zifencei, which I don't think you can just
>>> assume to be present?
>> Based on the specification:
>> ```
>> Chapter 34. RV32/64G Instruction Set Listings
>> One goal of the RISC-V project is that it be used as a stable software development target. For this
>> purpose, we define a combination of a base ISA (RV32I or RV64I) plus selected standard extensions
>> (IMAFD, Zicsr, Zifencei) as a "general-purpose" ISA, and we use the abbreviation G for the
>> IMAFDZicsr_Zifencei combination of instruction-set extensions. This chapter presents opcode maps
>> and instruction-set listings for RV32G and RV64G
>> ```
> Hmm, indeed. That's well hidden in a place I didn't expect it to live at.
> Maybe worth a sentence in the description?

Sure, I will update the description. ( and probably add the comment above invalidate_icache() function )

>> and that G is needed to boot Linux kernel ( and so Xen ) I make an assumption that Zifencei will be always
>> present.
> I'd be a little careful here. Xen may be used in Linux-free environments.
> I notice arch.mk specifies rv64g, yet I'm uncertain we shouldn't relax
> that at some point.
>
>> And based on Linux code (https://elixir.bootlin.com/linux/v6.12.4/source/arch/riscv/kernel/cpufeature.c#L676 )
>> when 'i' is present in riscv,isa property zifencei is present unconditionally.
> That looks questionable to me. I don't think Zifencei can be inferred from
> I. Historically it was, and imo that's what the comment there says. Plus
> it is dependent upon acpi_disabled.

Agree with you here. And it was the reason why I dropped this if-condition when I ported cpufeature.c to Xen.



~ Oleksii

[-- Attachment #2: Type: text/html, Size: 7477 bytes --]

[PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Oleksii Kurochko]

relocate_fdt() relocates FDT to Xen heap instead of using early mapping
as it is expected that discard_initial_modules() ( is supposed to call
in the future ) discards the FDT boot module and remove_early_mappings()
destroys the early mapping.

To implement that the following things are introduced as they are called
by internals of xmalloc_bytes() which is used in relocate_fdt():
1. As RISC-V may have non-coherent access for RAM ( f.e., in case
   of non-coherent IO devices ) flush_page_to_ram() is implemented
   to ensure that cache and RAM are consistent for such platforms.
2. copy_from_paddr() to copy FDT from a physical address to allocated
   by xmalloc_bytes() in Xen heap.
3. virt_to_page() to convert virtual address to page. Also introduce
   directmap_virt_end to check that VA argument of virt_to_page() is
   inside directmap region.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/mm.h   | 10 ++++++--
 xen/arch/riscv/include/asm/page.h | 10 ++++++--
 xen/arch/riscv/mm.c               |  3 +++
 xen/arch/riscv/setup.c            | 41 +++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 699ed23f0d..25fd38531f 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -8,11 +8,13 @@
 #include <xen/const.h>
 #include <xen/mm-frame.h>
 #include <xen/pdx.h>
+#include <xen/pfn.h>
 #include <xen/types.h>
 
 #include <asm/page-bits.h>
 
 extern vaddr_t directmap_virt_start;
+extern vaddr_t directmap_virt_end;
 
 #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
@@ -148,8 +150,12 @@ static inline void *page_to_virt(const struct page_info *pg)
 /* Convert between Xen-heap virtual addresses and page-info structures. */
 static inline struct page_info *virt_to_page(const void *v)
 {
-    BUG_ON("unimplemented");
-    return NULL;
+    unsigned long va = (unsigned long)v;
+
+    ASSERT(va >= DIRECTMAP_VIRT_START);
+    ASSERT(va <= directmap_virt_end);
+
+    return frametable_virt_start + PFN_DOWN(va - directmap_virt_start);
 }
 
 /*
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 0f297141d3..c245a4273f 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -7,6 +7,7 @@
 
 #include <xen/bug.h>
 #include <xen/const.h>
+#include <xen/domain_page.h>
 #include <xen/errno.h>
 #include <xen/types.h>
 
@@ -172,10 +173,15 @@ static inline void invalidate_icache(void)
 #define clear_page(page) memset((void *)(page), 0, PAGE_SIZE)
 #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
 
-/* TODO: Flush the dcache for an entire page. */
 static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 {
-    BUG_ON("unimplemented");
+    void *v = map_domain_page(_mfn(mfn));
+
+    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
+    unmap_domain_page(v);
+
+    if ( sync_icache )
+        invalidate_icache();
 }
 
 /* Write a pagetable entry. */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index f2bf279bac..c614d547a6 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -419,6 +419,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 }
 
 vaddr_t __ro_after_init directmap_virt_start = DIRECTMAP_VIRT_START;
+vaddr_t __ro_after_init directmap_virt_end;
 
 struct page_info *__ro_after_init frametable_virt_start = frame_table;
 
@@ -556,6 +557,8 @@ void __init setup_mm(void)
         setup_directmap_mappings(PFN_DOWN(bank_start), PFN_DOWN(bank_size));
     }
 
+    directmap_virt_end = directmap_virt_start + ram_end - 1;
+
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
 }
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 9680332fee..ff667260ec 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -12,6 +12,7 @@
 #include <public/version.h>
 
 #include <asm/early_printk.h>
+#include <asm/fixmap.h>
 #include <asm/sbi.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
@@ -26,6 +27,46 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
+/**
+ * copy_from_paddr - copy data from a physical address
+ * @dst: destination virtual address
+ * @paddr: source physical address
+ * @len: length to copy
+ */
+void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+{
+    void *src = (void *)FIXMAP_ADDR(FIX_MISC);
+
+    while (len) {
+        unsigned long l, s;
+
+        s = paddr & (PAGE_SIZE - 1);
+        l = min(PAGE_SIZE - s, len);
+
+        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
+        memcpy(dst, src + s, l);
+        clean_dcache_va_range(dst, l);
+        clear_fixmap(FIX_MISC);
+
+        paddr += l;
+        dst += l;
+        len -= l;
+    }
+}
+
+/* Relocate the FDT in Xen heap */
+static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
+{
+    void *fdt = xmalloc_bytes(dtb_size);
+
+    if ( !fdt )
+        panic("Unable to allocate memory for relocating the Device-Tree.\n");
+
+    copy_from_paddr(fdt, dtb_paddr, dtb_size);
+
+    return fdt;
+}
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
-- 
2.47.0

Re: [PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Jan Beulich]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> relocate_fdt() relocates FDT to Xen heap instead of using early mapping
> as it is expected that discard_initial_modules() ( is supposed to call
> in the future ) discards the FDT boot module and remove_early_mappings()
> destroys the early mapping.
> 
> To implement that the following things are introduced as they are called
> by internals of xmalloc_bytes() which is used in relocate_fdt():
> 1. As RISC-V may have non-coherent access for RAM ( f.e., in case
>    of non-coherent IO devices ) flush_page_to_ram() is implemented
>    to ensure that cache and RAM are consistent for such platforms.

This is a detail of the page allocator, yes. It can then be viewed as also
a detail of xmalloc() et al, but I consider the wording a little misleading.

> 2. copy_from_paddr() to copy FDT from a physical address to allocated
>    by xmalloc_bytes() in Xen heap.

This doesn't look to be related to the internals of xmalloc() et al.

> 3. virt_to_page() to convert virtual address to page. Also introduce
>    directmap_virt_end to check that VA argument of virt_to_page() is
>    inside directmap region.

This is a need of free_xenheap_pages(), yes; see remark on point 1.

> @@ -148,8 +150,12 @@ static inline void *page_to_virt(const struct page_info *pg)
>  /* Convert between Xen-heap virtual addresses and page-info structures. */
>  static inline struct page_info *virt_to_page(const void *v)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    unsigned long va = (unsigned long)v;
> +
> +    ASSERT(va >= DIRECTMAP_VIRT_START);
> +    ASSERT(va <= directmap_virt_end);

Why the difference compared to virt_to_maddr()?

Also recall my comment on one of your earlier series, regarding inclusive
vs exclusive ranges. Can that please be sorted properly as a prereq, to
avoid extending the inconsistency?

> @@ -172,10 +173,15 @@ static inline void invalidate_icache(void)
>  #define clear_page(page) memset((void *)(page), 0, PAGE_SIZE)
>  #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
>  
> -/* TODO: Flush the dcache for an entire page. */
>  static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
>  {
> -    BUG_ON("unimplemented");
> +    void *v = map_domain_page(_mfn(mfn));

const void *?

> +    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> +    unmap_domain_page(v);
> +
> +    if ( sync_icache )
> +        invalidate_icache();
>  }
>  
>  /* Write a pagetable entry. */
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -419,6 +419,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  }
>  
>  vaddr_t __ro_after_init directmap_virt_start = DIRECTMAP_VIRT_START;
> +vaddr_t __ro_after_init directmap_virt_end;

If the variable is needed (see above) it pretty certainly wants an
initializer, too.

> @@ -26,6 +27,46 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +/**
> + * copy_from_paddr - copy data from a physical address
> + * @dst: destination virtual address
> + * @paddr: source physical address
> + * @len: length to copy
> + */
> +void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)

Without a declaration in a header this function ought to be static.

> +{
> +    void *src = (void *)FIXMAP_ADDR(FIX_MISC);

const void *

> +    while (len) {

Nit: Style.

> +        unsigned long l, s;
> +
> +        s = paddr & (PAGE_SIZE - 1);
> +        l = min(PAGE_SIZE - s, len);

Make these the variables' initializers?

> +        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
> +        memcpy(dst, src + s, l);
> +        clean_dcache_va_range(dst, l);

Why is this necessary here? You're copying to plain RAM that Xen alone
is using.

> +/* Relocate the FDT in Xen heap */
> +static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)

This function having no caller will - aiui - mean build breakage at
this point of the series.

> +{
> +    void *fdt = xmalloc_bytes(dtb_size);

New code ought to be using xvmalloc() et al. Unless there's a firm
reason not to.

Jan

Re: [PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 4812 bytes --]


On 12/9/24 4:00 PM, Jan Beulich wrote:
> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>> relocate_fdt() relocates FDT to Xen heap instead of using early mapping
>> as it is expected that discard_initial_modules() ( is supposed to call
>> in the future ) discards the FDT boot module and remove_early_mappings()
>> destroys the early mapping.
>>
>> To implement that the following things are introduced as they are called
>> by internals of xmalloc_bytes() which is used in relocate_fdt():
>> 1. As RISC-V may have non-coherent access for RAM ( f.e., in case
>>     of non-coherent IO devices ) flush_page_to_ram() is implemented
>>     to ensure that cache and RAM are consistent for such platforms.
> This is a detail of the page allocator, yes. It can then be viewed as also
> a detail of xmalloc() et al, but I consider the wording a little misleading.
>
>> 2. copy_from_paddr() to copy FDT from a physical address to allocated
>>     by xmalloc_bytes() in Xen heap.
> This doesn't look to be related to the internals of xmalloc() et al.
>
>> 3. virt_to_page() to convert virtual address to page. Also introduce
>>     directmap_virt_end to check that VA argument of virt_to_page() is
>>     inside directmap region.
> This is a need of free_xenheap_pages(), yes; see remark on point 1.

Actually I faced the usage of virt_to_page() in xmalloc_whole_page():
```
   static void *xmalloc_whole_pages(unsigned long size, unsigned long align)
   {
     ...
     PFN_ORDER(virt_to_page(res)) = PFN_UP(size);
     /* Check that there was no truncation: */
     ASSERT(PFN_ORDER(virt_to_page(res)) == PFN_UP(size));

     return res;
   }
```
which is called from xmalloc().

Do we need a second paragraph of the commit message at all? Or it is just obvious if
flush_page_to_ram(), virt_to_page() and copy_from_paddr() are introduces that they are needed for
relocate_fdt()?

Or perhaps rephrasing in the following way would be enough?
```
For internal use of|xmalloc|, the functions|flush_page_to_ram()| and|virt_to_page()| are introduced.
|virt_to_page()| is also required for|free_xenheap_pages()|. These additions are used to support
|xmalloc|, which is utilized within|relocate_fdt()|. Additionally,|copy_from_paddr()| is introduced
for use in|relocate_fdt()|.
```

>> @@ -148,8 +150,12 @@ static inline void *page_to_virt(const struct page_info *pg)
>>   /* Convert between Xen-heap virtual addresses and page-info structures. */
>>   static inline struct page_info *virt_to_page(const void *v)
>>   {
>> -    BUG_ON("unimplemented");
>> -    return NULL;
>> +    unsigned long va = (unsigned long)v;
>> +
>> +    ASSERT(va >= DIRECTMAP_VIRT_START);
>> +    ASSERT(va <= directmap_virt_end);
> Why the difference compared to virt_to_maddr()?

It is just a mistake as `directmap_virt_end` is directmap_virt_start-relative but `v` is DIRECTMAP_VIRT_START-relative.
The check should be following:
   ASSERT((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END));
and then directmap_virt_end should be dropped at all.

>
> Also recall my comment on one of your earlier series, regarding inclusive
> vs exclusive ranges. Can that please be sorted properly as a prereq, to
> avoid extending the inconsistency?

Yes, I remember that and at the moment everything ( DIRECTMAP_VIRT_END, FRAMETABLE_VIRT_END )
is following "inclusive" way. Considering that you remind me that could you please tell me more time
what I am missing?

>> +        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
>> +        memcpy(dst, src + s, l);
>> +        clean_dcache_va_range(dst, l);
> Why is this necessary here? You're copying to plain RAM that Xen alone
> is using.

It is Arm specific:
```
commit c60209d77e2c02de110ca0fdaa2582ef4e53d8fd
Author: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
Date:   Mon Jan 21 12:40:31 2013 +0000

     xen/arm: flush dcache after memcpy'ing the kernel image
     
     After memcpy'ing the kernel in guest memory we need to flush the dcache
     to make sure that the data actually reaches the memory before we start
     executing guest code with caches disabled.
     
     copy_from_paddr is the function that does the copy, so add a
     flush_xen_dcache_va_range there.
```
I wanted to put copy_from_paddr() to some common place at the end but in RISC-V cache is always enabled
( I don't see an instruction in the spec for disable/enable cache ) so this issue isn't present for RISC-V
and clean_dcache_va_range() should/could be dropped.

>> +/* Relocate the FDT in Xen heap */
>> +static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
> This function having no caller will - aiui - mean build breakage at
> this point of the series.

Yes, it should be a problem, missed that. Then I have to merge it with the next one patch.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 6858 bytes --]

Re: [PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Jan Beulich]

On 10.12.2024 16:20, Oleksii Kurochko wrote:
> On 12/9/24 4:00 PM, Jan Beulich wrote:
>> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>>> relocate_fdt() relocates FDT to Xen heap instead of using early mapping
>>> as it is expected that discard_initial_modules() ( is supposed to call
>>> in the future ) discards the FDT boot module and remove_early_mappings()
>>> destroys the early mapping.
>>>
>>> To implement that the following things are introduced as they are called
>>> by internals of xmalloc_bytes() which is used in relocate_fdt():
>>> 1. As RISC-V may have non-coherent access for RAM ( f.e., in case
>>>     of non-coherent IO devices ) flush_page_to_ram() is implemented
>>>     to ensure that cache and RAM are consistent for such platforms.
>> This is a detail of the page allocator, yes. It can then be viewed as also
>> a detail of xmalloc() et al, but I consider the wording a little misleading.
>>
>>> 2. copy_from_paddr() to copy FDT from a physical address to allocated
>>>     by xmalloc_bytes() in Xen heap.
>> This doesn't look to be related to the internals of xmalloc() et al.
>>
>>> 3. virt_to_page() to convert virtual address to page. Also introduce
>>>     directmap_virt_end to check that VA argument of virt_to_page() is
>>>     inside directmap region.
>> This is a need of free_xenheap_pages(), yes; see remark on point 1.
> 
> Actually I faced the usage of virt_to_page() in xmalloc_whole_page():
> ```
>    static void *xmalloc_whole_pages(unsigned long size, unsigned long align)
>    {
>      ...
>      PFN_ORDER(virt_to_page(res)) = PFN_UP(size);
>      /* Check that there was no truncation: */
>      ASSERT(PFN_ORDER(virt_to_page(res)) == PFN_UP(size));
> 
>      return res;
>    }
> ```
> which is called from xmalloc().
> 
> Do we need a second paragraph of the commit message at all? Or it is just obvious if
> flush_page_to_ram(), virt_to_page() and copy_from_paddr() are introduces that they are needed for
> relocate_fdt()?
> 
> Or perhaps rephrasing in the following way would be enough?
> ```
> For internal use of|xmalloc|, the functions|flush_page_to_ram()| and|virt_to_page()| are introduced.
> |virt_to_page()| is also required for|free_xenheap_pages()|. These additions are used to support
> |xmalloc|, which is utilized within|relocate_fdt()|. Additionally,|copy_from_paddr()| is introduced
> for use in|relocate_fdt()|.
> ```

I think that would do.

>> Also recall my comment on one of your earlier series, regarding inclusive
>> vs exclusive ranges. Can that please be sorted properly as a prereq, to
>> avoid extending the inconsistency?
> 
> Yes, I remember that and at the moment everything ( DIRECTMAP_VIRT_END, FRAMETABLE_VIRT_END )
> is following "inclusive" way. Considering that you remind me that could you please tell me more time
> what I am missing?

First the table azt the top of config.h uses all exclusive upper bounds.
And then DIRECTMAP_SIZE's definition assumes DIRECTMAP_SLOT_END would be
exclusive, when it's inclusive.

>>> +        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
>>> +        memcpy(dst, src + s, l);
>>> +        clean_dcache_va_range(dst, l);
>> Why is this necessary here? You're copying to plain RAM that Xen alone
>> is using.
> 
> It is Arm specific:
> ```
> commit c60209d77e2c02de110ca0fdaa2582ef4e53d8fd
> Author: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> Date:   Mon Jan 21 12:40:31 2013 +0000
> 
>      xen/arm: flush dcache after memcpy'ing the kernel image
>      
>      After memcpy'ing the kernel in guest memory we need to flush the dcache
>      to make sure that the data actually reaches the memory before we start
>      executing guest code with caches disabled.
>      
>      copy_from_paddr is the function that does the copy, so add a
>      flush_xen_dcache_va_range there.
> ```
> I wanted to put copy_from_paddr() to some common place at the end but in RISC-V cache is always enabled
> ( I don't see an instruction in the spec for disable/enable cache ) so this issue isn't present for RISC-V
> and clean_dcache_va_range() should/could be dropped.

That plus there's no kernel in sight just yet.

Jan

Re: [PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

On 12/10/24 5:20 PM, Jan Beulich wrote:
>>> Also recall my comment on one of your earlier series, regarding inclusive
>>> vs exclusive ranges. Can that please be sorted properly as a prereq, to
>>> avoid extending the inconsistency?
>> Yes, I remember that and at the moment everything ( DIRECTMAP_VIRT_END, FRAMETABLE_VIRT_END )
>> is following "inclusive" way. Considering that you remind me that could you please tell me more time
>> what I am missing?
> First the table azt the top of config.h uses all exclusive upper bounds.
> And then DIRECTMAP_SIZE's definition assumes DIRECTMAP_SLOT_END would be
> exclusive, when it's inclusive.

Really missed to update the tale on the top of config.h.

But it seems to me like any *_SIZE will be defined in exclusive way by its nature, doesn't it?
For example, size of directmap is (509-200+1)<<30 = 0x7F80000000 and it is not really (
0x7F80000000 - 1 ) = 7F7FFFFFFF.

I prefer to have DIRECTMAP_{SIZE,VIRT_END} defined as now:
   #define DIRECTMAP_SIZE          (SLOTN(DIRECTMAP_SLOT_END + 1) - SLOTN(DIRECTMAP_SLOT_START))
   #define DIRECTMAP_VIRT_END      (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
( of course with making upper bounds inclusive in the table on the top of config.h )

>
>>>> +        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
>>>> +        memcpy(dst, src + s, l);
>>>> +        clean_dcache_va_range(dst, l);
>>> Why is this necessary here? You're copying to plain RAM that Xen alone
>>> is using.
>> It is Arm specific:
>> ```
>> commit c60209d77e2c02de110ca0fdaa2582ef4e53d8fd
>> Author: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>> Date:   Mon Jan 21 12:40:31 2013 +0000
>>
>>       xen/arm: flush dcache after memcpy'ing the kernel image
>>       
>>       After memcpy'ing the kernel in guest memory we need to flush the dcache
>>       to make sure that the data actually reaches the memory before we start
>>       executing guest code with caches disabled.
>>       
>>       copy_from_paddr is the function that does the copy, so add a
>>       flush_xen_dcache_va_range there.
>> ```
>> I wanted to put copy_from_paddr() to some common place at the end but in RISC-V cache is always enabled
>> ( I don't see an instruction in the spec for disable/enable cache ) so this issue isn't present for RISC-V
>> and clean_dcache_va_range() should/could be dropped.
> That plus there's no kernel in sight just yet.

( clarification ) will it change something if kernel will be loaded now? It seems even if we are copying kernel in guest
memory we still don't need to flush the dcache as cache is enabled and cache coherence protocol will do a work automatically.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 3762 bytes --]

Re: [PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Jan Beulich]

On 11.12.2024 11:26, Oleksii Kurochko wrote:
> On 12/10/24 5:20 PM, Jan Beulich wrote:
>>>> Also recall my comment on one of your earlier series, regarding inclusive
>>>> vs exclusive ranges. Can that please be sorted properly as a prereq, to
>>>> avoid extending the inconsistency?
>>> Yes, I remember that and at the moment everything ( DIRECTMAP_VIRT_END, FRAMETABLE_VIRT_END )
>>> is following "inclusive" way. Considering that you remind me that could you please tell me more time
>>> what I am missing?
>> First the table azt the top of config.h uses all exclusive upper bounds.
>> And then DIRECTMAP_SIZE's definition assumes DIRECTMAP_SLOT_END would be
>> exclusive, when it's inclusive.
> 
> Really missed to update the tale on the top of config.h.
> 
> But it seems to me like any *_SIZE will be defined in exclusive way by its nature, doesn't it?

Of course. I'm not even sure "size" can be reasonably qualified as "exclusive" or
"inclusive".

> For example, size of directmap is (509-200+1)<<30 = 0x7F80000000 and it is not really (
> 0x7F80000000 - 1 ) = 7F7FFFFFFF.
> 
> I prefer to have DIRECTMAP_{SIZE,VIRT_END} defined as now:
>    #define DIRECTMAP_SIZE          (SLOTN(DIRECTMAP_SLOT_END + 1) - SLOTN(DIRECTMAP_SLOT_START))
>    #define DIRECTMAP_VIRT_END      (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
> ( of course with making upper bounds inclusive in the table on the top of config.h )

Right.

>>>>> +        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
>>>>> +        memcpy(dst, src + s, l);
>>>>> +        clean_dcache_va_range(dst, l);
>>>> Why is this necessary here? You're copying to plain RAM that Xen alone
>>>> is using.
>>> It is Arm specific:
>>> ```
>>> commit c60209d77e2c02de110ca0fdaa2582ef4e53d8fd
>>> Author: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>> Date:   Mon Jan 21 12:40:31 2013 +0000
>>>
>>>       xen/arm: flush dcache after memcpy'ing the kernel image
>>>       
>>>       After memcpy'ing the kernel in guest memory we need to flush the dcache
>>>       to make sure that the data actually reaches the memory before we start
>>>       executing guest code with caches disabled.
>>>       
>>>       copy_from_paddr is the function that does the copy, so add a
>>>       flush_xen_dcache_va_range there.
>>> ```
>>> I wanted to put copy_from_paddr() to some common place at the end but in RISC-V cache is always enabled
>>> ( I don't see an instruction in the spec for disable/enable cache ) so this issue isn't present for RISC-V
>>> and clean_dcache_va_range() should/could be dropped.
>> That plus there's no kernel in sight just yet.
> 
> ( clarification ) will it change something if kernel will be loaded now? It seems even if we are copying kernel in guest
> memory we still don't need to flush the dcache as cache is enabled and cache coherence protocol will do a work automatically.

Correct. My point merely was that there are two reasons this isn't needed, each
of which is by itself sufficient to justify omitting that call.

Jan

[PATCH v1 6/6] xen/riscv: relocating and unflattening host device tree

[Oleksii Kurochko]

Relocate FDT to Xen heap instead of using early mapping as it is
expected that discard_initial_modules() ( is supposed to call in
the future ) discards the FDT boot module and remove_early_mappings()
destroys the early mapping.

Unflatten a device tree, creating the tree of struct device_node.
It also fills the "name" and "type" pointers of the nodes so the normal
device-tree walking functions can be used.

Set device_tree_flattened to NULL in the case when acpi_disabled is
equal to false.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/setup.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index ff667260ec..41826f77fb 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/acpi.h>
 #include <xen/bug.h>
 #include <xen/bootfdt.h>
 #include <xen/compile.h>
@@ -71,6 +72,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
     const char *cmdline;
+    size_t fdt_size;
 
     remove_identity_mapping();
 
@@ -95,7 +97,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
                           _end - _start, false) )
         panic("Failed to add BOOTMOD_XEN\n");
 
-    if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
+    if ( !(fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr)) )
         BUG();
 
     cmdline = boot_fdt_cmdline(device_tree_flattened);
@@ -114,6 +116,18 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
      */
     system_state = SYS_STATE_boot;
 
+    if ( acpi_disabled )
+    {
+        printk("Booting using Device Tree\n");
+        device_tree_flattened = relocate_fdt(dtb_addr, fdt_size);
+        dt_unflatten_host_device_tree();
+    }
+    else
+    {
+        device_tree_flattened = NULL;
+        panic("Booting using ACPI isn't supported\n");
+    }
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.47.0

Re: [PATCH v1 6/6] xen/riscv: relocating and unflattening host device tree

[Jan Beulich]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> Relocate FDT to Xen heap instead of using early mapping as it is
> expected that discard_initial_modules() ( is supposed to call in
> the future ) discards the FDT boot module and remove_early_mappings()
> destroys the early mapping.
> 
> Unflatten a device tree, creating the tree of struct device_node.
> It also fills the "name" and "type" pointers of the nodes so the normal
> device-tree walking functions can be used.
> 
> Set device_tree_flattened to NULL in the case when acpi_disabled is
> equal to false.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Albeit ...

> @@ -71,6 +72,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>                                 paddr_t dtb_addr)
>  {
>      const char *cmdline;
> +    size_t fdt_size;
>  
>      remove_identity_mapping();
>  
> @@ -95,7 +97,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>                            _end - _start, false) )
>          panic("Failed to add BOOTMOD_XEN\n");
>  
> -    if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
> +    if ( !(fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr)) )
>          BUG();

... perhaps better

    fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr);
    BUG_ON(!fdt_size);

?

Jan

Re: [PATCH v1 6/6] xen/riscv: relocating and unflattening host device tree

[Jan Beulich]

On 09.12.2024 16:56, Jan Beulich wrote:
> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>> Relocate FDT to Xen heap instead of using early mapping as it is
>> expected that discard_initial_modules() ( is supposed to call in
>> the future ) discards the FDT boot module and remove_early_mappings()
>> destroys the early mapping.
>>
>> Unflatten a device tree, creating the tree of struct device_node.
>> It also fills the "name" and "type" pointers of the nodes so the normal
>> device-tree walking functions can be used.
>>
>> Set device_tree_flattened to NULL in the case when acpi_disabled is
>> equal to false.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Albeit ...
> 
>> @@ -71,6 +72,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>                                 paddr_t dtb_addr)
>>  {
>>      const char *cmdline;
>> +    size_t fdt_size;
>>  
>>      remove_identity_mapping();
>>  
>> @@ -95,7 +97,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>                            _end - _start, false) )
>>          panic("Failed to add BOOTMOD_XEN\n");
>>  
>> -    if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
>> +    if ( !(fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr)) )
>>          BUG();
> 
> ... perhaps better
> 
>     fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr);
>     BUG_ON(!fdt_size);
> 
> ?

And then I notice that Arm has no such check at all. Better stay in sync?

Jan

Re: [PATCH v1 6/6] xen/riscv: relocating and unflattening host device tree

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]


On 12/9/24 4:57 PM, Jan Beulich wrote:
> On 09.12.2024 16:56, Jan Beulich wrote:
>> On 27.11.2024 13:50, Oleksii Kurochko wrote:
>>> Relocate FDT to Xen heap instead of using early mapping as it is
>>> expected that discard_initial_modules() ( is supposed to call in
>>> the future ) discards the FDT boot module and remove_early_mappings()
>>> destroys the early mapping.
>>>
>>> Unflatten a device tree, creating the tree of struct device_node.
>>> It also fills the "name" and "type" pointers of the nodes so the normal
>>> device-tree walking functions can be used.
>>>
>>> Set device_tree_flattened to NULL in the case when acpi_disabled is
>>> equal to false.
>>>
>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> Acked-by: Jan Beulich<jbeulich@suse.com>
>>
>> Albeit ...
>>
>>> @@ -71,6 +72,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>>                                  paddr_t dtb_addr)
>>>   {
>>>       const char *cmdline;
>>> +    size_t fdt_size;
>>>   
>>>       remove_identity_mapping();
>>>   
>>> @@ -95,7 +97,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>>                             _end - _start, false) )
>>>           panic("Failed to add BOOTMOD_XEN\n");
>>>   
>>> -    if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
>>> +    if ( !(fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr)) )
>>>           BUG();
>> ... perhaps better
>>
>>      fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr);
>>      BUG_ON(!fdt_size);
>>
>> ?
> And then I notice that Arm has no such check at all. Better stay in sync?

Agree, it is better to be in sync. I will drop the BUG_ON().

Thanks.

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 2552 bytes --]