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

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Tue, Oct 22, 2019 at 9:14 PM Julien Grall <julien.grall@xxxxxxx> wrote:
>
> 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.
>

Yes. So, either  ..._gen_json should generate entries for default
values or ..._parse_json
should set missing entries to its default values. Second solution looks more
correct.

> >
> > 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.

Thanks. I will post the separate path.

>
> Cheers,
>
> --
> Julien Grall



-- 
Best Regards,
Oleksandr Grytsov.

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