Re: [Xen-devel] [PATCH 1/3] xenstore: setup xenstore stubdom console interface properly

["Jürgen Groß" <jgross@suse.com>]

On 11.02.20 21:18, Andrew Cooper wrote:
> On 28/01/2020 14:28, Juergen Gross wrote:
>> In order to be able to get access to the console of Xenstore stubdom
>> we need an appropriate granttab entry. So call xc_dom_gnttab_init()
>> when constructing the domain and preset some information needed
>> for that function in the dom structure.
>>
>> We need to create the event channel for the console, too. Do that and
>> store all necessary data locally.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/helpers/init-xenstore-domain.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
>> index adb8408b63..a312bc38b8 100644
>> --- a/tools/helpers/init-xenstore-domain.c
>> +++ b/tools/helpers/init-xenstore-domain.c
>> @@ -24,6 +24,8 @@ static char *param;
>>   static char *name = "Xenstore";
>>   static int memory;
>>   static int maxmem;
>> +static xen_pfn_t console_mfn;
>> +static unsigned int console_evtchn;
>>   
>>   static struct option options[] = {
>>       { "kernel", 1, NULL, 'k' },
>> @@ -113,6 +115,7 @@ static int build(xc_interface *xch)
>>           fprintf(stderr, "xc_domain_setmaxmem failed\n");
>>           goto err;
>>       }
>> +    console_evtchn = xc_evtchn_alloc_unbound(xch, domid, 0);
> 
> Presumably some form of error checking?

Yes.

> 
> Also, while it is probably fine for now, I think this does highlight a
> future issue.  What happens when xenconsoled is also a stubdomain?
> 
> I suspect we have a looming chicken&egg problem, where the toolstack is
> going to have to do some careful domid and plumbing to set up build both
> stubdoms in tandem.

Hmm, I'd rather defer console setup in xenstore-stubdom then and do it
later via a XS_CONTROL message. This will even enable a restart of
console-stubdom.

> 
>>       rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
>>       if ( rv )
>>       {
>> @@ -133,6 +136,9 @@ static int build(xc_interface *xch)
>>           snprintf(cmdline, 512, "--event %d --internal-db", rv);
>>   
>>       dom = xc_dom_allocate(xch, cmdline, NULL);
> 
> Any chance of some error handling, unlikely as it is to go wrong?

Okay.

> 
>> +    dom->container_type = XC_DOM_PV_CONTAINER;
>> +    dom->xenstore_domid = domid;
>> +    dom->console_evtchn = console_evtchn;
> 
> and a newline here?

Okay.

> 
>>       rv = xc_dom_kernel_file(dom, kernel);
>>       if ( rv )
>>       {
>> @@ -186,6 +192,12 @@ static int build(xc_interface *xch)
>>           fprintf(stderr, "xc_dom_boot_image failed\n");
>>           goto err;
>>       }
>> +    rv = xc_dom_gnttab_init(dom);
>> +    if ( rv )
>> +    {
>> +        fprintf(stderr, "xc_dom_gnttab_init failed\n");
>> +        goto err;
>> +    }
>>   
>>       rv = xc_domain_set_virq_handler(xch, domid, VIRQ_DOM_EXC);
>>       if ( rv )
>> @@ -201,6 +213,7 @@ static int build(xc_interface *xch)
>>       }
>>   
>>       rv = 0;
>> +    console_mfn = xc_dom_p2m(dom, dom->console_pfn);
> 
> This doesn't appear to be used.

Oh, the usage is in patch 2. Probably I should move this addition to
that patch then.


Juergen

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