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

Re: [Xen-devel] [PATCH 2/4] stubdom/vtpm: Add reconfiguration support



On 11/29/2012 01:53 PM, Matthew Fioravante wrote:
> The purpose of this is to allow 2 entities in the same vm to use tpm drivers, 
> pv_grub and the linux guest. The Xenbus Reconfigure states are new to me. Is 
> this intended behavior in line with the original purpose of the Reconfigure 
> states or are we hijacking them to do something not in the original xen 
> front/back driver spec?
> 
> It looks like pv-grub just shuts down blkfront and the others without any 
> reconfigure magic. Given that vtpm-stubdom will no longer automatically 
> shutdown with your later patch, is there any reason we cannot just do the 
> same here?

The vtpm backend in mini-os cleans up after itself destructively when it is
closed: it removes xenstore entries and frees the data structures, and so
requires the xenstore directory to be re-created. I originally wanted to keep
the shutdown semantics the same (to preserve the vtpm auto-shutdown), although
that's not reflected in this patch queue.

I think the correct method here is to make the backend act like it does for the
reconfigure on close, and trigger free only when the xenstore entry vanishes.
 
> On 11/27/2012 10:14 AM, Daniel De Graaf wrote:
>> Allow the vtpm device to be disconnected and reconnected so that a
>> bootloader (like pv-grub) can submit measurements and return the vtpm
>> device to its initial state before booting the target kernel.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>   extras/mini-os/include/tpmfront.h |  2 +-
>>   extras/mini-os/lib/sys.c          |  2 +-
>>   extras/mini-os/tpmback.c          |  5 +++++
>>   extras/mini-os/tpmfront.c         | 15 +++++++++------
>>   stubdom/vtpm/vtpm.c               |  2 +-
>>   5 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/extras/mini-os/include/tpmfront.h 
>> b/extras/mini-os/include/tpmfront.h
>> index a0c7c4d..913faa4 100644
>> --- a/extras/mini-os/include/tpmfront.h
>> +++ b/extras/mini-os/include/tpmfront.h
>> @@ -61,7 +61,7 @@ struct tpmfront_dev {
>>   /*Initialize frontend */
>>   struct tpmfront_dev* init_tpmfront(const char* nodename);
>>   /*Shutdown frontend */
>> -void shutdown_tpmfront(struct tpmfront_dev* dev);
>> +void shutdown_tpmfront(struct tpmfront_dev* dev, int for_reconfig);
>>     /* Send a tpm command to the backend and wait for the response
>>    *
>> diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
>> index 3cc3340..03da4f0 100644
>> --- a/extras/mini-os/lib/sys.c
>> +++ b/extras/mini-os/lib/sys.c
>> @@ -459,7 +459,7 @@ int close(int fd)
>>   #endif
>>   #ifdef CONFIG_TPMFRONT
>>       case FTYPE_TPMFRONT:
>> -            shutdown_tpmfront(files[fd].tpmfront.dev);
>> +            shutdown_tpmfront(files[fd].tpmfront.dev, 0);
>>           files[fd].type = FTYPE_NONE;
>>           return 0;
>>   #endif
>> diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c
>> index 2d31061..ea42235 100644
>> --- a/extras/mini-os/tpmback.c
>> +++ b/extras/mini-os/tpmback.c
>> @@ -664,6 +664,7 @@ static int frontend_changed(tpmif_t* tpmif)
>>      switch (state) {
>>         case XenbusStateInitialising:
>>         case XenbusStateInitialised:
>> +      case XenbusStateReconfigured:
>>        break;
>>           case XenbusStateConnected:
>> @@ -678,6 +679,10 @@ static int frontend_changed(tpmif_t* tpmif)
>>        tpmif_change_state(tpmif, XenbusStateClosing);
>>        break;
>>   +      case XenbusStateReconfiguring:
>> +         disconnect_fe(tpmif);
>> +     break;
>> +
>>         case XenbusStateUnknown: /* keep it here */
>>         case XenbusStateClosed:
>>        free_tpmif(tpmif);
>> diff --git a/extras/mini-os/tpmfront.c b/extras/mini-os/tpmfront.c
>> index c1cbab3..b725ba0 100644
>> --- a/extras/mini-os/tpmfront.c
>> +++ b/extras/mini-os/tpmfront.c
>> @@ -344,10 +344,10 @@ struct tpmfront_dev* init_tpmfront(const char* 
>> _nodename)
>>      return dev;
>>     error:
>> -   shutdown_tpmfront(dev);
>> +   shutdown_tpmfront(dev, 0);
>>      return NULL;
>>   }
>> -void shutdown_tpmfront(struct tpmfront_dev* dev)
>> +void shutdown_tpmfront(struct tpmfront_dev* dev, int for_reconfig)
> It might be cleaner to create a new function like reconfigure_tpmfront() or 
> something like that instead of adding an option to shutdown_tpmfront().
>>   {
>>      char* err;
>>      char path[512];
>> @@ -357,8 +357,7 @@ void shutdown_tpmfront(struct tpmfront_dev* dev)
>>      TPMFRONT_LOG("Shutting down tpmfront%s\n", for_reconfig ? " for 
>> reconfigure" : "");
>>      /* disconnect */
>>      if(dev->state == XenbusStateConnected) {
>> -      dev->state = XenbusStateClosing;
>> -      //FIXME: Transaction for this?
>> +      dev->state = for_reconfig ? XenbusStateReconfiguring : 
>> XenbusStateClosing;
>>         /* Tell backend we are closing */
>>         if((err = xenbus_printf(XBT_NIL, dev->nodename, "state", "%u", 
>> (unsigned int) dev->state))) {
>>        free(err);
>> @@ -374,15 +373,19 @@ void shutdown_tpmfront(struct tpmfront_dev* dev)
>>        free(err);
>>         }
>>   +      if (for_reconfig)
>> +         wait_for_backend_state_changed(dev, XenbusStateReconfigured);
>> +
>>         /* Tell backend we are closed */
>> -      dev->state = XenbusStateClosed;
>> +      dev->state = for_reconfig ? XenbusStateInitialising : 
>> XenbusStateClosed;
>>         if((err = xenbus_printf(XBT_NIL, dev->nodename, "state", "%u", 
>> (unsigned int) dev->state))) {
>>        TPMFRONT_ERR("Unable to write to %s, error was %s", dev->nodename, 
>> err);
>>        free(err);
>>         }
>>           /* Wait for the backend to close and unmap shared pages, ignore 
>> any errors */
>> -      wait_for_backend_state_changed(dev, XenbusStateClosed);
>> +      if (!for_reconfig)
>> +         wait_for_backend_state_changed(dev, XenbusStateClosed);
>>           /* Close event channel and unmap shared page */
>>         mask_evtchn(dev->evtchn);
>> diff --git a/stubdom/vtpm/vtpm.c b/stubdom/vtpm/vtpm.c
>> index 71aef78..c33e078 100644
>> --- a/stubdom/vtpm/vtpm.c
>> +++ b/stubdom/vtpm/vtpm.c
>> @@ -394,7 +394,7 @@ abort_postvtpmblk:
>>   abort_postrng:
>>        /* Close devices */
>> -   shutdown_tpmfront(tpmfront_dev);
>> +   shutdown_tpmfront(tpmfront_dev, 0);
>>   abort_posttpmfront:
>>      shutdown_tpmback();
>>   
> 
> 


-- 
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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