Re: [PATCH v4 1/9] xen/riscv: dt_processor_hartid() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.06.2025 10:26, Oleksii Kurochko wrote:
> 
> On 6/10/25 4:08 PM, Jan Beulich wrote:
>> On 05.06.2025 17:58, Oleksii Kurochko wrote:
>>> @@ -14,3 +17,77 @@ void __init smp_prepare_boot_cpu(void)
>>>       cpumask_set_cpu(0, &cpu_possible_map);
>>>       cpumask_set_cpu(0, &cpu_online_map);
>>>   }
>>> +
>>> +/**
>>> + * dt_get_hartid - Get the hartid from a CPU device node
>>> + *
>>> + * @cpun: CPU number(logical index) for which device node is required
>>> + *
>>> + * Return: The hartid for the CPU node or ~0UL if not found.
>>> + */
>>> +static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
>>> +{
>>> +    const __be32 *cell;
>>> +    unsigned int ac;
>>> +    uint32_t len;
>>> +    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
>>> +
>>> +    ac = dt_n_addr_cells(cpun);
>>> +    cell = dt_get_property(cpun, "reg", &len);
>>> +
>>> +    if (ac > max_cells) {
>> Besides the (double) style issue, why's this needed? Can't you simply ...
>>
>>> +        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
>>> +               max_cells);
>>> +        return ~0UL;
>>> +    }
>>> +
>>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
>> ... write the last part here in a way that there can't be overflow?
>> ac > len / sizeof(*cell) that is? (Remaining question then is what to
>> do when len isn't evenly divisible by sizeof(*cell).)
> 
> reg property should be always evenly divisible by sizeof(*cell) according to 
> device
> tree binding:
>    The reg property describes the address of the device’s resources within 
> the address space defined by its parent bus. Most commonly this means 
> the offsets and lengths of memory-mapped IO register blocks, but may 
> have a different meaning on some bus types. Addresses in the address 
> space defined by the root node are CPU real addresses.
>    
>    The value is a <prop-encoded-array>, composed of an arbitrary number of 
> pairs of address and length, <address length>. The number of <u32> cells 
> required to specify the address and length are bus-specific and are 
> specified by the #address-cells and #size-cells properties in the parent 
> of the device node. If the parent node specifies a value of 0 for 
> #size-cells, the length field in the value of reg shall be omitted. So 
> it is guaranteed by DTC compiler and it would be enough to check 
> overflow in suggested by you way: ac > len / sizeof(*cell)
> But considering what you noticed below ...
> 
>>
>>> +        return ~0UL;
>>> +
>>> +    return dt_read_number(cell, ac);
>> What meaning does this have for ac > 2? (As per your checking above
>> it can be up to UINT32_MAX / 4.)
> 
> ... It will be an issue for dt_read_number() which could deal only with 
> uint64_t what means
> we can't have ac > 2. (UINT32_MAX / 4 it is a theoretical maximum IIUC)
> 
> Thereby we could do in the following way:
> @@ -30,19 +30,18 @@ static unsigned long dt_get_hartid(const struct 
> dt_device_node *cpun)
>       const __be32 *cell;
>       unsigned int ac;
>       uint32_t len;
> -    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
>   
>       ac = dt_n_addr_cells(cpun);
>       cell = dt_get_property(cpun, "reg", &len);
>   
> -    if (ac > max_cells) {
> -        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
> -               max_cells);
> +    if ( !cell || !ac || (ac > len / sizeof(*cell)) )
>           return ~0UL;
> -    }
>   
> -    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> -        return ~0UL;
> +    /*
> +     * If ac > 2, the result may be truncated or meaningless unless
> +     * dt_read_number() supports wider integers.
> +     */
> +    BUG_ON(ac > 2);
>   
>       return dt_read_number(cell, ac);
>   }
> 
> I am not sure that BUG_ON() should be in dt_get_hartid(). Probably it would 
> be better move it
> to dt_read_number() as if one day support for RV128 will be needed I assume 
> that it will be
> needed to change a prototype of dt_read_number() to work with address-cells = 
> 3.
> What do you think? Could I go with the suggested above changes or it would be 
> better to move
> BUG_ON() to dt_read_number()?

Don't know; the DT maintainers would have to judge. I don't, however, think it 
should
be BUG_ON() - as said several times before, that's a check suitable to cover for
possible mistakes in Xen code. Here however you're trying to cover for a flaw 
in DT.

Jan