Re: [PATCH v3 5/5] tools: add example application to initialize dom0less PV drivers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 28 Mar 2022, Julien Grall wrote:
> On 23/03/2022 02:50, Stefano Stabellini wrote:
> > On Sat, 29 Jan 2022, Julien Grall wrote:
> > > On 28/01/2022 21:33, Stefano Stabellini wrote:
> > > > +    libxl_uuid uuid;
> > > > +    uint64_t v;
> > > > +    int rc;
> > > > +
> > > > +    printf("Init dom0less domain: %d\n", info->domid);
> > > > +    dom.guest_domid = info->domid;
> > > > +    dom.xenstore_domid = 0;
> > > > +    dom.xch = xc_interface_open(0, 0, 0);
> > > > +
> > > > +    rc = xc_hvm_param_get(dom.xch, info->domid, HVM_PARAM_STORE_EVTCHN,
> > > > &v);
> > > > +    if (rc != 0) {
> > > > +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
> > > > +        return 1;
> > > > +    }
> > > > +    dom.xenstore_evtchn = v;
> > > > +
> > > > +    /* Console won't be initialized but set its data for completeness
> > > > */
> > > > +    dom.console_domid = 0;
> > > 
> > > I find a bit odd you set the domid but not the event channel, page. Can
> > > you
> > > explain?
> > > 
> > > Actually, can you explain why only half of the structure is initialized?
> >   We are only using the struct xc_dom_image parameter for
> > xc_dom_gnttab_init, and nothing else. We only need very few fields in
> > it. Basically we could call xc_dom_gnttab_seed directly and then we
> > wouldn't need struct xc_dom_image at all. Only the needed fields are
> > currently populated.
> 
> I would prefer if we don't use xc_dom_image and call xc_dom_gnttab_seed().
> This would:
>   1) reduce the risk that one of the unitialized field is will be mistakenly
> use
>   2) extra documentation (currently missing) to explain which fields is used.

I have done that, it is in v4


> > > > +
> > > > +    /* Alloc magic pages */
> > > > +    if (alloc_magic_pages(info, &dom) != 0) {
> > > > +        printf("Error on alloc magic pages\n");
> > > > +        return 1;
> > > > +    }
> > > > +
> > > > +    xc_dom_gnttab_init(&dom);
> > > 
> > > This call as the risk to break the guest if the dom0 Linux doesn't support
> > > the
> > > acquire interface. This is because it will punch a hole in the domain
> > > memory
> > > where the grant-table may have already been mapped.
> > > 
> > > Also, this function could fails.
> > 
> > I'll check for return errors. Dom0less is for fully static
> > configurations so I think it is OK to return error and abort if
> > something unexpected happens: dom0less' main reason for being is that
> > there is nothing unexpected :-)
> Does this mean the caller will have to reboot the system if there is an error?
> IOW, we don't expect them to call ./init-dom0less twice.

Yes, exactly. I think init-dom0less could even panic. My mental model is
that this is an "extension" of construct_domU. Over there we just panic
if something is wrong and here it would be similar. The user provided a
wrong config and should fix it.


> > > > +
> > > > +    libxl_uuid_generate(&uuid);
> > > > +    xc_domain_sethandle(dom.xch, info->domid,
> > > > libxl_uuid_bytearray(&uuid));
> > > > +
> > > > +    rc = gen_stub_json_config(info->domid, &uuid);
> > > > +    if (rc)
> > > > +        err(1, "gen_stub_json_config");
> > > > +
> > > > +    rc = restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
> > > > +    if (rc)
> > > > +        err(1, "writing to xenstore");
> > > > +
> > > > +    xs_introduce_domain(xsh, info->domid,
> > > > +            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
> > > > +            dom.xenstore_evtchn);
> > > 
> > > xs_introduce_domain() can technically fails.
> > 
> > OK
> > 
> > 
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +/* Check if domain has been configured in XS */
> > > > +static bool domain_exists(struct xs_handle *xsh, int domid)
> > > > +{
> > > > +    return xs_is_domain_introduced(xsh, domid);
> > > > +}
> > > 
> > > Would not this lead to initialize a domain with PV driver disabled?
> > 
> > I am not sure I understood your question, but I'll try to answer anyway.
> > This check is purely to distinguish dom0less guests, which needs further
> > initializations, from regular guests (e.g. xl guests) that don't need
> > any actions taken here.
> 
> Dom0less domUs can be divided in two categories based on whether they are xen
> aware (e.g. xen,enhanced is set).
> 
> Looking at this script, it seems to assume that all dom0less domUs are Xen
> aware. So it will end up to allocate Xenstore ring and call
> xs_introduce_domain(). I suspect the call will end up to fail because the
> event channel would be 0.
> 
> So did you try to use this script on a platform where there only xen aware
> domU and/or a mix?

Good idea of asking for this test. I thought I already ran that test,
but I did it again to be sure. Everything works OK (although the
xenstore page allocation is unneeded). xs_introduce_domain does not
fail: I think that's because it is usually called on all domains by the
toolstack, even the ones without xenstore support in the kernel.