Re: [PATCH v2 7/8] xen/common: dom0less: introduce common domain-build.c

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2025 17:26, Oleksii Kurochko wrote:
> 
> On 4/17/25 4:45 PM, Jan Beulich wrote:
>> On 14.04.2025 17:56, Oleksii Kurochko wrote:
>>> --- a/xen/include/xen/fdt-domain-build.h
>>> +++ b/xen/include/xen/fdt-domain-build.h
>>> @@ -5,6 +5,7 @@
>>>   #include <xen/bootfdt.h>
>>>   #include <xen/device_tree.h>
>>>   #include <xen/fdt-kernel.h>
>>> +#include <xen/mm.h>
>>>   #include <xen/types.h>
>>>   
>>>   #if __has_include(<asm/domain_build.h>)
>>> @@ -32,7 +33,37 @@ int make_memory_node(const struct kernel_info *kinfo, 
>>> int addrcells,
>>>                        int sizecells, const struct membanks *mem);
>>>   int make_timer_node(const struct kernel_info *kinfo);
>>>   
>>> -unsigned int get_allocation_size(paddr_t size);
>>> +
>>> +static inline int get_allocation_size(paddr_t size)
>>> +{
>>> +    /*
>>> +     * get_order_from_bytes returns the order greater than or equal to
>>> +     * the given size, but we need less than or equal. Adding one to
>>> +     * the size pushes an evenly aligned size into the next order, so
>>> +     * we can then unconditionally subtract 1 from the order which is
>>> +     * returned.
>>> +     */
>>> +    return get_order_from_bytes(size + 1) - 1;
>>> +}
>>> +
>>> +typedef unsigned long (*copy_to_guest_phys_cb)(struct domain *d,
>>> +                                               paddr_t gpa,
>>> +                                               void *buf,
>> This very much looks like the latest now, when the code is made common,
>> it wants to be const void *. Even if this may require another prereq
>> patch.
>>
>> However, instead of using a function pointer, couldn't the now common
>> code call copy_to_guest_phys_flush_dcache() conditionally upon
>> IS_ENABLED(CONFIG_<whatever>)?
> 
> I thought about having IS_ENABLED(CONFIG_ARM) as, at the moment, this is 
> necessary to
> be called only for Arm as guest domain on Arm could be ran with cache 
> disabled so to be sure
> that DTB, kernel and initrd is fully in RAM this function should be called.
> 
> For RISC-V, it isn't possible case as guest domain won't run with cache 
> disabled.
> 
>> Or provide a weak
>> copy_to_guest_phys_flush_dcache() which would simply call
>> copy_to_guest_phys()?
> 
> Could it be a weak function with empty implementation?

Not really, no, as the fallback has to work for the case where cache
management isn't explicitly necessary.

> Is copy_to_guest_phys() implemented for other archs?

What "other" are you thinking about here? An arch wanting to use this
code would need to provide one. But x86, for example, isn't going to
build this code aiui, and hence has no need for such a function. We
have hvm_copy_to_guest_phys() there.

Hmm, but I notice only now that Arm has no plain copy_to_guest_phys().
This certainly breaks the fallback idea I had outlined. So perhaps I
should recommend that you stick to the function pointer approach for
now, unless Arm folks come up with any good suggestion. (I notice PPC
has a (stub) copy_to_guest_phys_flush_dcache(), too; it's unclear to
me whether that's really needed there, or whether it isn't more like
RISC-V in this regard.)

Jan