| Message ID | 1421159133-31526-23-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 29/01/15 11:07, Stefano Stabellini wrote: > On Tue, 13 Jan 2015, Julien Grall wrote: >> The partial device tree may contains phandle. The Device Tree Compiler >> tends to allocate the phandle from 1. >> >> Reserve the ID 65000 for the GIC phandle. I think we can safely assume >> that the partial device tree will never contain a such ID. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Wei Liu <wei.liu2@citrix.com> >> > > Shouldn't we at least check that the partial device tree doesn't contain > a conflicting phandle? I don't think so. This will unlikely happen, and if it happens the guest will crash with an obvious error. Regards,
On 29/01/15 12:28, Stefano Stabellini wrote: > On Thu, 29 Jan 2015, Julien Grall wrote: >> On 29/01/15 11:07, Stefano Stabellini wrote: >>> On Tue, 13 Jan 2015, Julien Grall wrote: >>>> The partial device tree may contains phandle. The Device Tree Compiler >>>> tends to allocate the phandle from 1. >>>> >>>> Reserve the ID 65000 for the GIC phandle. I think we can safely assume >>>> that the partial device tree will never contain a such ID. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>> >>> >>> Shouldn't we at least check that the partial device tree doesn't contain >>> a conflicting phandle? >> >> I don't think so. This will unlikely happen, and if it happens the guest >> will crash with an obvious error. > > It is good that the error is obvious. > > But how expensive is to check for it? I would have to check the validity of the properties (name + value size). At least the properties "linux,phandle" and "phandle" should be checked. Though I could do in copy_properties but I find it hackish. > Think about the poor user that ends up in this situation: the fact that > is unlikely only makes it harder for a user to figure out what to do to > fix it. The poor use will have to write his device tree by hand to hit this error ;). So using the right phandle is not a huge drawback. Regards,
On 29/01/15 15:04, Stefano Stabellini wrote: > On Thu, 29 Jan 2015, Julien Grall wrote: >> On 29/01/15 12:28, Stefano Stabellini wrote: >>> On Thu, 29 Jan 2015, Julien Grall wrote: >>>> On 29/01/15 11:07, Stefano Stabellini wrote: >>>>> On Tue, 13 Jan 2015, Julien Grall wrote: >>>>>> The partial device tree may contains phandle. The Device Tree Compiler >>>>>> tends to allocate the phandle from 1. >>>>>> >>>>>> Reserve the ID 65000 for the GIC phandle. I think we can safely assume >>>>>> that the partial device tree will never contain a such ID. >>>>>> >>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>>>> >>>>> >>>>> Shouldn't we at least check that the partial device tree doesn't contain >>>>> a conflicting phandle? >>>> >>>> I don't think so. This will unlikely happen, and if it happens the guest >>>> will crash with an obvious error. >>> >>> It is good that the error is obvious. >>> >>> But how expensive is to check for it? >> >> I would have to check the validity of the properties (name + value >> size). At least the properties "linux,phandle" and "phandle" should be >> checked. >> >> Though I could do in copy_properties but I find it hackish. >> >>> Think about the poor user that ends up in this situation: the fact that >>> is unlikely only makes it harder for a user to figure out what to do to >>> fix it. >> >> The poor use will have to write his device tree by hand to hit this >> error ;). >> >> So using the right phandle is not a huge drawback. > > Fair enough. Please document this limitation in the docs and/or manuals. I will do. Regards,
On 23/02/2015 14:36, Ian Campbell wrote: > On Thu, 2015-01-29 at 13:48 +0000, Julien Grall wrote: >> On 29/01/15 12:28, Stefano Stabellini wrote: >>> On Thu, 29 Jan 2015, Julien Grall wrote: >>>> On 29/01/15 11:07, Stefano Stabellini wrote: >>>>> On Tue, 13 Jan 2015, Julien Grall wrote: >>>>>> The partial device tree may contains phandle. The Device Tree Compiler >>>>>> tends to allocate the phandle from 1. >>>>>> >>>>>> Reserve the ID 65000 for the GIC phandle. I think we can safely assume >>>>>> that the partial device tree will never contain a such ID. >>>>>> >>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>>>> >>>>> >>>>> Shouldn't we at least check that the partial device tree doesn't contain >>>>> a conflicting phandle? >>>> >>>> I don't think so. This will unlikely happen, and if it happens the guest >>>> will crash with an obvious error. >>> >>> It is good that the error is obvious. >>> >>> But how expensive is to check for it? >> >> I would have to check the validity of the properties (name + value >> size). At least the properties "linux,phandle" and "phandle" should be >> checked. >> >> Though I could do in copy_properties but I find it hackish. > > Can't you just track the largest phandle ever seen during > copy_properties and then use N+1 for the GIC? Now the we decided to trust the input device tree, it would be easier to write the code. I will give a look. > >>> Think about the poor user that ends up in this situation: the fact that >>> is unlikely only makes it harder for a user to figure out what to do to >>> fix it. >> >> The poor use will have to write his device tree by hand to hit this >> error ;). > > Or use a new version of dtc which does things differently for some > reason. And you would not be able to get a phandle for the GIC if largest phandle is too high. So the guest won't work correctly. Regards,
Hi Ian, On 24/02/2015 10:08, Ian Campbell wrote: > On Mon, 2015-02-23 at 22:02 +0000, Julien Grall wrote: >> >> On 23/02/2015 14:36, Ian Campbell wrote: >>> On Thu, 2015-01-29 at 13:48 +0000, Julien Grall wrote: >>>> On 29/01/15 12:28, Stefano Stabellini wrote: >>>>> On Thu, 29 Jan 2015, Julien Grall wrote: >>>>>> On 29/01/15 11:07, Stefano Stabellini wrote: >>>>>>> On Tue, 13 Jan 2015, Julien Grall wrote: >>>>>>>> The partial device tree may contains phandle. The Device Tree Compiler >>>>>>>> tends to allocate the phandle from 1. >>>>>>>> >>>>>>>> Reserve the ID 65000 for the GIC phandle. I think we can safely assume >>>>>>>> that the partial device tree will never contain a such ID. >>>>>>>> >>>>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>>>>>> >>>>>>> >>>>>>> Shouldn't we at least check that the partial device tree doesn't contain >>>>>>> a conflicting phandle? >>>>>> >>>>>> I don't think so. This will unlikely happen, and if it happens the guest >>>>>> will crash with an obvious error. >>>>> >>>>> It is good that the error is obvious. >>>>> >>>>> But how expensive is to check for it? >>>> >>>> I would have to check the validity of the properties (name + value >>>> size). At least the properties "linux,phandle" and "phandle" should be >>>> checked. >>>> >>>> Though I could do in copy_properties but I find it hackish. >>> >>> Can't you just track the largest phandle ever seen during >>> copy_properties and then use N+1 for the GIC? >> >> Now the we decided to trust the input device tree, it would be easier to >> write the code. >> >> I will give a look. >> >>> >>>>> Think about the poor user that ends up in this situation: the fact that >>>>> is unlikely only makes it harder for a user to figure out what to do to >>>>> fix it. >>>> >>>> The poor use will have to write his device tree by hand to hit this >>>> error ;). >>> >>> Or use a new version of dtc which does things differently for some >>> reason. >> >> And you would not be able to get a phandle for the GIC if largest >> phandle is too high. >> >> So the guest won't work correctly. > > Indeed, filling in a bitmap as you go might be another option, although > you'd either need an 8k bitmap (not so bad in userspace) or to realloc > as it grows. I though about different implementation for tracking the phandle. All require to parse the partial device tree twice: one for tracking the phandle and the second for copy the nodes. This is because we need to have the GIC phandle at the time we create the root node (properties has to be created before the child nodes). So I plan to keep this patch as it is and documenting the value of the GIC. Would it be fine for you? Though I could put an higher value too. Regards,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 619458b..dc745fb 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -78,10 +78,11 @@ static struct arch_info { {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" }, }; -enum { - PHANDLE_NONE = 0, - PHANDLE_GIC, -}; +/* + * The device tree compiler (DTC) is allocating the phandle from 1 to + * onwards. Reserve a high value for the GIC phandle. + */ +#define PHANDLE_GIC (65000) typedef uint32_t be32; typedef be32 gic_interrupt[3];
The partial device tree may contains phandle. The Device Tree Compiler tends to allocate the phandle from 1. Reserve the ID 65000 for the GIC phandle. I think we can safely assume that the partial device tree will never contain a such ID. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- Changes in v3: - Patch added --- tools/libxl/libxl_arm.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)