[Xen-devel] [PATCH v1] libxl: Add DTB compatible list to config file

Oleksandr Grytsov posted 1 patch 6 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20191010141231.25363-1-al1img@gmail.com
tools/libxl/libxl_arm.c     | 42 ++++++++++++++++++++++++++++++-------
tools/libxl/libxl_types.idl |  1 +
tools/xl/xl_parse.c         |  7 +++++++
3 files changed, 42 insertions(+), 8 deletions(-)

[Xen-devel] [PATCH v1] libxl: Add DTB compatible list to config file

Posted by Oleksandr Grytsov 6 days ago
From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Some platforms need more compatible property values in device
tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
values that are given by Xen by default.
Specify in domain configuration file which values should be added
by providing "dtb_compatible" list of strings separated by comas.

Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 tools/libxl/libxl_arm.c     | 42 ++++++++++++++++++++++++++++++-------
 tools/libxl/libxl_types.idl |  1 +
 tools/xl/xl_parse.c         |  7 +++++++
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index bf31b9b3ca..b956a6356c 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -270,20 +270,46 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
 
 static int make_root_properties(libxl__gc *gc,
                                 const libxl_version_info *vers,
-                                void *fdt)
+                                void *fdt,
+                                const libxl_domain_build_info *info)
 {
-    int res;
+    const char *compat0 = GCSPRINTF("xen,xenvm-%d.%d",
+                                    vers->xen_version_major,
+                                    vers->xen_version_minor);
+    const char *compat1 = "xen,xenvm";
+    const char **compats;
+    char *compat, *p;
+    size_t sz = 0;
+    int i, res, num_compats;
 
     res = fdt_property_string(fdt, "model", GCSPRINTF("XENVM-%d.%d",
                                                       vers->xen_version_major,
                                                       vers->xen_version_minor));
     if (res) return res;
 
-    res = fdt_property_compat(gc, fdt, 2,
-                              GCSPRINTF("xen,xenvm-%d.%d",
-                                        vers->xen_version_major,
-                                        vers->xen_version_minor),
-                              "xen,xenvm");
+    num_compats = 2 + libxl_string_list_length(&info->dt_compatible);
+    compats = libxl__zalloc(gc, num_compats * sizeof(*compats));
+    if (!compats)
+        return -FDT_ERR_INTERNAL;
+
+    compats[0] = compat0;
+    compats[1] = compat1;
+    sz = strlen(compat0) + strlen(compat1) + 2;
+    for (i = 0; info->dt_compatible && info->dt_compatible[i] != NULL; i++) {
+        compats[2 + i] = info->dt_compatible[i];
+        sz += strlen(info->dt_compatible[i]) + 1;
+    }
+
+    p = compat = libxl__zalloc(gc, sz);
+    if (!p)
+        return -FDT_ERR_INTERNAL;
+
+    for (i = 0; i < num_compats; i++) {
+        strcpy(p, compats[i]);
+        p += strlen(compats[i]) + 1;
+    }
+
+    res = fdt_property(fdt, "compatible", compat, sz);
     if (res) return res;
 
     res = fdt_property_cell(fdt, "interrupt-parent", GUEST_PHANDLE_GIC);
@@ -930,7 +956,7 @@ next_resize:
 
         FDT( fdt_begin_node(fdt, "") );
 
-        FDT( make_root_properties(gc, vers, fdt) );
+        FDT( make_root_properties(gc, vers, fdt, info) );
         FDT( make_chosen_node(gc, fdt, !!dom->modules[0].blob, state, info) );
         FDT( make_cpus_node(gc, fdt, info->max_vcpus, ainfo) );
         FDT( make_psci_node(gc, fdt) );
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 3ac9494b80..08ffb65904 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -544,6 +544,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     # Note that the partial device tree should avoid to use the phandle
     # 65000 which is reserved by the toolstack.
     ("device_tree",      string),
+    ("dt_compatible",    libxl_string_list),
     ("acpi",             libxl_defbool),
     ("bootloader",       string),
     ("bootloader_args",  libxl_string_list),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 03a2c54dd2..db9821c765 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2408,6 +2408,13 @@ skip_vfb:
         }
     }
 
+    e = xlu_cfg_get_list_as_string_list(config, "dt_compatible",
+                                        &b_info->dt_compatible, 1);
+    if (e && e != ESRCH) {
+            fprintf(stderr,"xl: Unable to parse dt_compatible\n");
+            exit(-ERROR_FAIL);
+    }
+
     if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) {
         d_config->num_usbctrls = 0;
         d_config->usbctrls = NULL;
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v1] libxl: Add DTB compatible list to config file

Posted by Ian Jackson 4 days ago
Oleksandr Grytsov writes ("[PATCH v1] libxl: Add DTB compatible list to config file"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Some platforms need more compatible property values in device
> tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
> values that are given by Xen by default.
> Specify in domain configuration file which values should be added
> by providing "dtb_compatible" list of strings separated by comas.

Hi, thanks.

I don't have an opinion about the principle of this and would like to
hear from ARM folks about that.

Also, Stefano, Julien: should we be asking for a freeze exception for
this for 4.13 ?


As for the detail of the code:

The method you use for building compats[] is really rather ad-hoc.
Why not use a flexarray ?

(Also you do not need to check the error return from libxl__zalloc.
From libxl.h:
 * Memory allocation failures are not handled gracefully.  If malloc
 * (or realloc) fails, libxl will cause the entire process to print
 * a message to stderr and exit with status 255.
But really (i) you should be using GCNEW_ARRAY anyway and (ii) this
is all irrelevant if you switch to a flexarray instead.)

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v1] libxl: Add DTB compatible list to config file

Posted by Julien Grall 4 days ago
Hi,

On 11/10/2019 16:23, Ian Jackson wrote:
> Oleksandr Grytsov writes ("[PATCH v1] libxl: Add DTB compatible list to config file"):
>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>>
>> Some platforms need more compatible property values in device
>> tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
>> values that are given by Xen by default.

I am pretty sure I have seen this patch a few years ago, but I can't find it in 
my inbox. What is the exact problem here?

>> Specify in domain configuration file which values should be added
>> by providing "dtb_compatible" list of strings separated by comas.
> 
> Hi, thanks.
> 
> I don't have an opinion about the principle of this and would like to
> hear from ARM folks about that.
> 
> Also, Stefano, Julien: should we be asking for a freeze exception for
> this for 4.13 ?

I don't have enough context to understand the exact issue he is trying to solve.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v1] libxl: Add DTB compatible list to config file

Posted by Stefano Stabellini 4 days ago
On Fri, 11 Oct 2019, Julien Grall wrote:
> Hi,
> 
> On 11/10/2019 16:23, Ian Jackson wrote:
> > Oleksandr Grytsov writes ("[PATCH v1] libxl: Add DTB compatible list to
> > config file"):
> > > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > > 
> > > Some platforms need more compatible property values in device
> > > tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
> > > values that are given by Xen by default.
> 
> I am pretty sure I have seen this patch a few years ago, but I can't find it
> in my inbox. What is the exact problem here?
> 
> > > Specify in domain configuration file which values should be added
> > > by providing "dtb_compatible" list of strings separated by comas.
> > 
> > Hi, thanks.
> > 
> > I don't have an opinion about the principle of this and would like to
> > hear from ARM folks about that.
> > 
> > Also, Stefano, Julien: should we be asking for a freeze exception for
> > this for 4.13 ?
> 
> I don't have enough context to understand the exact issue he is trying to
> solve.

Same here. Is this patch needed because on some platform the OS checks
for the platform "model" (also known as "machine name") on device tree
before continuing or to trigger a difference behavior?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v1] libxl: Add DTB compatible list to config file

Posted by Oleksandr Grytsov 51 minutes ago
On Fri, Oct 11, 2019 at 8:21 PM Stefano Stabellini
<sstabellini@kernel.org> wrote:
>
> On Fri, 11 Oct 2019, Julien Grall wrote:
> > Hi,
> >
> > On 11/10/2019 16:23, Ian Jackson wrote:
> > > Oleksandr Grytsov writes ("[PATCH v1] libxl: Add DTB compatible list to
> > > config file"):
> > > > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > > >
> > > > Some platforms need more compatible property values in device
> > > > tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
> > > > values that are given by Xen by default.
> >
> > I am pretty sure I have seen this patch a few years ago, but I can't find it
> > in my inbox. What is the exact problem here?
> >
> > > > Specify in domain configuration file which values should be added
> > > > by providing "dtb_compatible" list of strings separated by comas.
> > >
> > > Hi, thanks.
> > >
> > > I don't have an opinion about the principle of this and would like to
> > > hear from ARM folks about that.
> > >
> > > Also, Stefano, Julien: should we be asking for a freeze exception for
> > > this for 4.13 ?
> >
> > I don't have enough context to understand the exact issue he is trying to
> > solve.
>
> Same here. Is this patch needed because on some platform the OS checks
> for the platform "model" (also known as "machine name") on device tree
> before continuing or to trigger a difference behavior?

Yes, exactly.

I will redo the patch with Ian's comments if it is ok in general.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v1] libxl: Add DTB compatible list to config file

Posted by Julien Grall 43 minutes ago
Hi Oleksandr,

On 16/10/2019 15:04, Oleksandr Grytsov wrote:
> On Fri, Oct 11, 2019 at 8:21 PM Stefano Stabellini
> <sstabellini@kernel.org> wrote:
>>
>> On Fri, 11 Oct 2019, Julien Grall wrote:
>>> Hi,
>>>
>>> On 11/10/2019 16:23, Ian Jackson wrote:
>>>> Oleksandr Grytsov writes ("[PATCH v1] libxl: Add DTB compatible list to
>>>> config file"):
>>>>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>>>>>
>>>>> Some platforms need more compatible property values in device
>>>>> tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
>>>>> values that are given by Xen by default.
>>>
>>> I am pretty sure I have seen this patch a few years ago, but I can't find it
>>> in my inbox. What is the exact problem here?
>>>
>>>>> Specify in domain configuration file which values should be added
>>>>> by providing "dtb_compatible" list of strings separated by comas.
>>>>
>>>> Hi, thanks.
>>>>
>>>> I don't have an opinion about the principle of this and would like to
>>>> hear from ARM folks about that.
>>>>
>>>> Also, Stefano, Julien: should we be asking for a freeze exception for
>>>> this for 4.13 ?
>>>
>>> I don't have enough context to understand the exact issue he is trying to
>>> solve.
>>
>> Same here. Is this patch needed because on some platform the OS checks
>> for the platform "model" (also known as "machine name") on device tree
>> before continuing or to trigger a difference behavior?
> 
> Yes, exactly.
> 
> I will redo the patch with Ian's comments if it is ok in general.

By specifying a different compatible (let say "renesas,r8a774a1"), then you 
claim that your virtual machine is exactly the same as that board.

This means, the OS is free to assume that the memory layout and all the devices 
are exactly the same. This is definitely not true as the virtual machine we 
expose is specific to Xen.

So I don't think this is the correct approach here. Can you provide a real-life 
example on why you need this?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v1] libxl: Add DTB compatible list to config file

Posted by Oleksandr Grytsov 20 minutes ago
On Wed, Oct 16, 2019 at 5:12 PM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Oleksandr,
>
> On 16/10/2019 15:04, Oleksandr Grytsov wrote:
> > On Fri, Oct 11, 2019 at 8:21 PM Stefano Stabellini
> > <sstabellini@kernel.org> wrote:
> >>
> >> On Fri, 11 Oct 2019, Julien Grall wrote:
> >>> Hi,
> >>>
> >>> On 11/10/2019 16:23, Ian Jackson wrote:
> >>>> Oleksandr Grytsov writes ("[PATCH v1] libxl: Add DTB compatible list to
> >>>> config file"):
> >>>>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >>>>>
> >>>>> Some platforms need more compatible property values in device
> >>>>> tree root node in addition to "xen,xenvm-%d.%d" and "xen,xenvm"
> >>>>> values that are given by Xen by default.
> >>>
> >>> I am pretty sure I have seen this patch a few years ago, but I can't find it
> >>> in my inbox. What is the exact problem here?
> >>>
> >>>>> Specify in domain configuration file which values should be added
> >>>>> by providing "dtb_compatible" list of strings separated by comas.
> >>>>
> >>>> Hi, thanks.
> >>>>
> >>>> I don't have an opinion about the principle of this and would like to
> >>>> hear from ARM folks about that.
> >>>>
> >>>> Also, Stefano, Julien: should we be asking for a freeze exception for
> >>>> this for 4.13 ?
> >>>
> >>> I don't have enough context to understand the exact issue he is trying to
> >>> solve.
> >>
> >> Same here. Is this patch needed because on some platform the OS checks
> >> for the platform "model" (also known as "machine name") on device tree
> >> before continuing or to trigger a difference behavior?
> >
> > Yes, exactly.
> >
> > I will redo the patch with Ian's comments if it is ok in general.
>
> By specifying a different compatible (let say "renesas,r8a774a1"), then you
> claim that your virtual machine is exactly the same as that board.
>
> This means, the OS is free to assume that the memory layout and all the devices
> are exactly the same. This is definitely not true as the virtual machine we
> expose is specific to Xen.
>
> So I don't think this is the correct approach here. Can you provide a real-life
> example on why you need this?
>
> Cheers,
>
> --
> Julien Grall

This is required for HW domains. Some drivers or initialization routines check
compatible. Below is example from linux kernel sources:

linux/sound/ppc/awacs.c:741:    if (of_machine_is_compatible("PowerBook3,1")
linux/sound/ppc/awacs.c:742:        ||
of_machine_is_compatible("PowerBook3,2")) {
linux/sound/ppc/awacs.c:770:#define IS_PM7500
(of_machine_is_compatible("AAPL,7500") \
linux/sound/ppc/awacs.c:771:        || of_machine_is_compatible("AAPL,8500") \
linux/sound/ppc/awacs.c:772:        || of_machine_is_compatible("AAPL,9500"))
...
linux/arch/arm/mach-omap2/pdata-quirks.c:703:        if
(of_machine_is_compatible(quirks->compatible)) {
linux/arch/arm/mach-omap2/pdata-quirks.c:717:    if
(of_machine_is_compatible("ti,omap2420") ||
linux/arch/arm/mach-omap2/pdata-quirks.c:718:
of_machine_is_compatible("ti,omap3"))
linux/arch/arm/mach-omap2/pdata-quirks.c:721:    if
(of_machine_is_compatible("ti,omap3"))
...

Also see [1]

[1] https://source.codeaurora.org/external/imx/imx-xen/commit/?h=imx_4.14.98_2.0.0_ga&id=6e58d478203639e71da3da69ffeae3fa5dc0197b

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel