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

[Matthew Fioravante <matthew.fioravante@jhuapl.edu>]

[-- Attachment #1.1: Type: text/plain, Size: 6981 bytes --]

On 11/29/2012 02:28 PM, Daniel De Graaf wrote:
> 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.
A cursory look at blkback in linux and it looks like the backend driver 
doesn't tear down the xenstore entries at all. I think its supposed to 
be the job of libxl to do that no? So I think I agree with you that what 
we really want is to change the backend to leave xenstore alone and 
maintain enough state so that the device can reconnect later.
>   
>> 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@tycho.nsa.gov>
>>> ---
>>>    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();
>>>    
>>
>



[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 1459 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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