Re: [PATCH 11/21] mini-os: make frontends and xenbus optional

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 01/23/2012 07:51 AM, Ian Campbell wrote:
> On Fri, 2012-01-20 at 20:47 +0000, Daniel De Graaf wrote:
>> This adds compile-time logic to disable certain frontends in mini-os:
>>  - pcifront is disabled by default, enabled for ioemu
>>  - blkfront, netfront, fbfront, and kbdfront are enabled by default
>>  - xenbus is required for any frontend, and is enabled by default
>>
>> If all frontends and xenbus are disabled, mini-os will run without
>> needing to communicate with xenstore, making it suitable to run the
>> xenstore daemon.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  extras/mini-os/Makefile               |    5 +++-
>>  extras/mini-os/apps/common.mk         |   11 +++++++++
>>  extras/mini-os/apps/ioemu.mk          |    1 +
>>  extras/mini-os/console/xencons_ring.c |   15 ++++++++++--
>>  extras/mini-os/files.mk               |   12 +++++-----
>>  extras/mini-os/include/lib.h          |    2 +
>>  extras/mini-os/kernel.c               |   40 +++++++++++++++++++++++++++++++-
>>  extras/mini-os/lib/sys.c              |   28 +++++++++++++++++++++++
>>  extras/mini-os/main.c                 |    6 +++-
>>  9 files changed, 106 insertions(+), 14 deletions(-)
>>
>> diff --git a/extras/mini-os/Makefile b/extras/mini-os/Makefile
>> index af7d0d4..7419211 100644
>> --- a/extras/mini-os/Makefile
>> +++ b/extras/mini-os/Makefile
>> @@ -70,7 +70,10 @@ ifeq ($(lwip),y)
>>  LWC    := $(shell find $(LWIPDIR)/ -type f -name '*.c')
>>  LWC    := $(filter-out %6.c %ip6_addr.c %ethernetif.c, $(LWC))
>>  LWO    := $(patsubst %.c,%.o,$(LWC))
>> -LWO    += $(addprefix $(OBJ_DIR)/,lwip-arch.o lwip-net.o)
>> +LWO    += $(OBJ_DIR)/lwip-arch.o
>> +ifeq ($(CONFIG_NETFRONT),y)
>> +LWO += $(OBJ_DIR)/lwip-net.o
>> +endif
> 
> Without lwip-net.o is there any point in having the rest of LWO? Or does
> the linker optimise it all away anyway?
> 

Some of the other parts of LWO may be useful on their own, depending on the
application run under minios. The xenstored configuration disables all of
LWO, so this doesn't matter there; the vTPM stub domains (from patches sent
by Matthew Fioravante in March 2011) do use parts of LWO without needing 
netfront support, which prompted this configuration test.

> [...]
> 
>> diff --git a/extras/mini-os/console/xencons_ring.c b/extras/mini-os/console/xencons_ring.c
>> index af0afed..c3eba35 100644
>> --- a/extras/mini-os/console/xencons_ring.c
>> +++ b/extras/mini-os/console/xencons_ring.c
>> @@ -189,6 +189,7 @@ struct consfront_dev *xencons_ring_init(void)
>>
>>  void free_consfront(struct consfront_dev *dev)
>>  {
>> +#ifdef CONFIG_XENBUS
>>      char* err = NULL;
>>      XenbusState state;
>>
>> @@ -217,6 +218,7 @@ void free_consfront(struct consfront_dev *dev)
>>  close:
>>      if (err) free(err);
>>      xenbus_unwatch_path_token(XBT_NIL, path, path);
>> +#endif
>>
>>      mask_evtchn(dev->evtchn);
>>      unbind_evtchn(dev->evtchn);
>> @@ -231,16 +233,18 @@ close:
>>
>>  struct consfront_dev *init_consfront(char *_nodename)
>>  {
>> +    struct consfront_dev *dev;
>> +    char nodename[256];
>> +    static int consfrontends = 3;
>> +#ifdef CONFIG_XENBUS
>>      xenbus_transaction_t xbt;
>>      char* err;
>>      char* message=NULL;
>>      int retry=0;
>>      char* msg = NULL;
>> -    char nodename[256];
>>      char path[256];
>> -    static int consfrontends = 3;
>> -    struct consfront_dev *dev;
>>      int res;
>> +#endif
>>
>>      if (!_nodename)
>>          snprintf(nodename, sizeof(nodename), "device/console/%d", consfrontends);
>> @@ -257,6 +261,7 @@ struct consfront_dev *init_consfront(char *_nodename)
>>      dev->fd = -1;
>>  #endif
>>
>> +#ifdef CONFIG_XENBUS
>>      snprintf(path, sizeof(path), "%s/backend-id", nodename);
>>      if ((res = xenbus_read_integer(path)) < 0)
>>          return NULL;
>> @@ -351,17 +356,21 @@ done:
>>              goto error;
>>          }
>>      }
>> +#endif
> 
> Haven't you ifdef'd out everything which would have set dev->evtchn?

Hmm, I might have. Will address it in the cleanup from the second mail.

> 
> I'm not sure that the CONFIG_XENBUS is worthwhile, at least at the
> moment, and it seems to add an awful lot of ifdefery.
> 
> [...]
>> diff --git a/extras/mini-os/kernel.c b/extras/mini-os/kernel.c
>> index 2875bf1..9e490d5 100644
>> --- a/extras/mini-os/kernel.c
>> +++ b/extras/mini-os/kernel.c
>> [...]
>> @@ -462,11 +474,21 @@ __attribute__((weak)) int app_main(start_info_t *si)
>>      printk("Dummy main: start_info=%p\n", si);
>>      create_thread("xenbus_tester", xenbus_tester, si);
>>      create_thread("periodic_thread", periodic_thread, si);
>> +#ifdef CONFIG_NETFRONT
>>      create_thread("netfront", netfront_thread, si);
>> +#endif
> 
> Better to define init_FOOfront for each of these and have it be a nop in
> the ifndef case and avoid the ifdefs in the code itself.
> 
> Likewise the ifdef's in the teardown. Ideally the actual meat in the
> ifdef cases would be moved into the files you aren't compiling (e.g.
> netfront_thread goes into netfront.c) and only the stubs remain in some
> header somewhere.
> 
> Ian.
> 

The majority of kernel.c seems to be test code intended to be overridden
by the application; see the comment above app_main:

/* This should be overridden by the application we are linked against. */
__attribute__((weak)) int app_main(start_info_t *si)

As such, maybe all of this code should be moved out of kernel.c and into
test.c, and have a better noop app_main in kernel.c.

-- 
Daniel De Graaf
National Security Agency