Re: [PATCH v2] xen/dt: Remove loop in dt_read_number()

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 18/06/2025 08:06, Orzel, Michal wrote:
> On 17/06/2025 19:13, Alejandro Vallejo wrote:
> > The DT spec declares only two number types for a property: u32 and u64,
> > as per Table 2.3 in Section 2.2.4. Remove unbounded loop and replace
> > with a switch statement. Default to a size of 1 cell in the nonsensical
> > size case, with a warning printed on the Xen console.
> >
> > Suggested-by: Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
> > ---
> > v2:
> >    * Added missing `break` on the `case 2:` branch and added 
> > ASSERT_UNREACHABLE() to the deafult path
> > ---
> >   xen/include/xen/device_tree.h | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 75017e4266..2ec668b94a 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -261,10 +261,21 @@ void intc_dt_preinit(void);
> >   /* Helper to read a big number; size is in cells (not bytes) */
> >   static inline u64 dt_read_number(const __be32 *cell, int size)
> >   {
> > -    u64 r = 0;
> > +    u64 r = be32_to_cpu(*cell);
> > +
> > +    switch ( size )
> > +    {
> > +    case 1:
> > +        break;
> > +    case 2:
> > +        r = (r << 32) | be32_to_cpu(cell[1]);
> > +        break;
> > +    default:
> > +        // Nonsensical size. default to 1.
> I wonder why there are so many examples of device trees in Linux with
> #address-cells = <3>? Also, libfdt defines FDT_MAX_NCELLS as 4 with comment:
> "maximum value for #address-cells and #size-cells" but I guess it follows the
> IEE1275 standard and DT spec "is loosely related" to it.
A lot of the use seems to be related to PCI. Below an example from [1]:

    pcie {
        #address-cells = <3>;
        #size-cells = <2>;

        pcie@0 {
            device_type = "pci";
            reg = <0x0 0x0 0x0 0x0 0x0>;
            #address-cells = <3>;
            #size-cells = <2>;
            ranges;

            wifi@0 {
                compatible = "pci17cb,1109";
                reg = <0x0 0x0 0x0 0x0 0x0>;

                qcom,calibration-variant = "RDP433_1";

                ports {
                    #address-cells = <1>;
                    #size-cells = <0>;

                    port@0 {
                        reg = <0>;

                        wifi1_wsi_tx: endpoint {
                            remote-endpoint = <&wifi2_wsi_rx>;
                        };
                    };

                    port@1 {
                        reg = <1>;

                        wifi1_wsi_rx: endpoint {
                            remote-endpoint = <&wifi3_wsi_tx>;
                        };
                    };
                };
            };
        };

"reg" has effectively 5 cells (3 for address and 2 for size).

From a very brief look, I couldn't find how this is interpreted. But I 
don't see how dt_read_number() can be used in this case. So I think it 
makes sense to restrict. The question though is whether it is a good 
idea to cap the value and behave differently from Linux.
Speaking of which there are another fun different behavior between Linux 
and Xen in DT. The default size of the root #address is 2 in Xen (see 
DT_ROOT_NODE_ADDR_CELLS_DEFAULT) which is spec compliant. But Linux will 
use 1 (see OF_ROOT_NODE_ADDR_CELLS_DEFAULT). I haven't seen any issue so 
far, but only notice recently when working on a separate project 
recently. I am still undecided for this one whether Xen should change 
because technically a Device-Tree should nowadays always provide 
#address-cells and #size-cells.
Cheers,

[1] devicetree/bindings/net/wireless/qcom,ath12k-wsi.yaml

--
Julien Grall