[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Stefano,

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.

+
+    /* 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.

+
+    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?

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.