Re: [Xen-devel] [XEN PATCH for-4.13 v1] Reset iomem's gfn to LIBXL_INVALID_GFN on reboot

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

Apologies for the late answer.

On 16/10/2019 14:09, Oleksandr Grytsov wrote:
> On Mon, Oct 14, 2019 at 12:28 PM Julien Grall <julien.grall@xxxxxxx> wrote:
> Thanks to point me out for this old thread. I completely forgot about it
> (I haven't worked with xen since long time). I've performed additional
> investigation
> and found the root cause of the issue. It doesn't relate to iomem GFN directly.
> The problem is in type from json parsing at place where libxl creates array of
> struct.
>
> For example, libxl_domain_config_from_json calls libxl_domain_config_init
> which initializes all child structures and arrays. But then when libxl parses
> json and creates the array of structure, it doesn't initialize array elements
> properly (see libxl__domain_build_info_parse_json iomem parsing):
>
> p->num_iomem = x->u.array->count;
> p->iomem = libxl__calloc(NOGC, p->num_iomem, sizeof(*p->iomem));
> if (!p->iomem && p->num_iomem != 0) {
>      rc = -1;
>      goto out;
> }
> for (i=0; (t=libxl__json_array_get(x,i)); i++) {
>      rc = libxl__iomem_range_parse_json(gc, t, &p->iomem[i]);
>      if (rc)
>         goto out;
> }
>
> libxl creates array element with calloc function, so all element
> fields are initialized
> with zero values. Even some of them have default value different from zero.
> For these purpose dedicated init function should be called for each element.
> Above example should be:
>
> for (i=0; (t=libxl__json_array_get(x,i)); i++) {
>      libxl_iomem_range_init(&p->iomem[i]);
>      rc = libxl__iomem_range_parse_json(gc, t, &p->iomem[i]);
>      if (rc)
>         goto out;
> }
Not initializing the values is fine as long as the JSON describes all the fields 
of the structure.
The key point here is the GFN is not described in the JSON (see 
libxl_iomem_range_gen_json) if it is equal to LIBXL_INVALID_GFN. As the field is 
not described, it will be defaulted to 0.
> I've changes gentypes.py as following:
>
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 88e5c5f30e..92e28be469 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -454,6 +454,8 @@ def libxl_C_type_parse_json(ty, w, v, indent = "
>   ", parent = None, discrimina
>           s += "        goto out;\n"
>           s += "    }\n"
>           s += "    for (i=0; (t=libxl__json_array_get(x,i)); i++) {\n"
> +        if ty.elem_type.init_fn is not None and
> ty.elem_type.autogenerate_init_fn:
My knowledge of libxl is quite limited. But I don't think this is correct, you 
want to call init_fn whether this has been autogenerated or not.
> +            s += indent + "    "+"%s_init(&%s[i]);\n" %
> (ty.elem_type.typename, v)
Looking at the other usage (like _libxl_C_type_init), init_fn is called with

            s += "%s(%s);\n" % (ty.init_fn, ty.pass_arg(v, parent is None))

I am also not entirely sure whether we should also cater the ty.init_val != None 
as well here.
>           s += libxl_C_type_parse_json(ty.elem_type, "t", v+"[i]",
>                                        indent + "    ", parent)
>           s += "    }\n"
>
> I'm not sure is it right and complete fix.
>
> Ian, could you review?
>
> If the fix is ok, I will submit the patch.
IHMO, the idea is there. The code may need some modifications (see above). 
Please post a patch so we can go forward in the process to review it.
Cheers,

--
Julien Grall

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