Re: [PATCH v3 14/16] x86/hyperlaunch: add memory parsing to domain config

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Thu Apr 10, 2025 at 1:03 PM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> @@ -212,6 +213,39 @@ static int __init process_domain_node(
>>              else
>>                  printk("PV\n");
>>          }
>> +        else if ( strncmp(prop_name, "memory", name_len) == 0 )
>> +        {
>> +            uint64_t kb;
>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>
> Nit (you know what I have to say here, and again below.)

Ack

>
>> +            {
>> +                printk("  failed processing memory for domain %s\n", name);
>> +                return -EINVAL;
>
> Any reason to override fdt_prop_as_u64()'s return value here?

Mostly to avoid needing to recover the error code. I'll just do it.

>
>> +            }
>> +            bd->mem_pages = PFN_DOWN(kb * SZ_1K);
>> +            printk("  memory: %ld kb\n", kb);
>> +        }
>> +        else if ( strncmp(prop_name, "mem-min", name_len) == 0 )
>> +        {
>> +            uint64_t kb;
>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>> +            {
>> +                printk("  failed processing memory for domain %s\n", name);
>> +                return -EINVAL;
>> +            }
>> +            bd->min_pages = PFN_DOWN(kb * SZ_1K);
>> +            printk("  min memory: %ld kb\n", kb);
>> +        }
>> +        else if ( strncmp(prop_name, "mem-max", name_len) == 0 )
>> +        {
>> +            uint64_t kb;
>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>> +            {
>> +                printk("  failed processing memory for domain %s\n", name);
>
> All three error messages being identical doesn't help diagnosing issues.

Indeed. Will add the prop that trigger each.

>
>> --- a/xen/include/xen/libfdt/libfdt-xen.h
>> +++ b/xen/include/xen/libfdt/libfdt-xen.h
>> @@ -34,6 +34,16 @@ static inline int __init fdt_prop_as_u32(
>>      return 0;
>>  }
>>  
>> +static inline int __init fdt_prop_as_u64(
>> +    const struct fdt_property *prop, uint64_t *val)
>> +{
>> +    if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) )
>> +        return -EINVAL;
>> +
>> +    *val = fdt_cell_as_u64((fdt32_t *)prop->data);
>
> Please avoid casting away const. Looks like I overlooked this in
> fdt_prop_as_u32() that was introduced by an earlier patch.

As part of v4 I moved this and fdt_prop_as_u32() earlier to patch8 and
already adjusted accordingly.

Cheers,
Alejandro