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

[Matthew Fioravante <matthew.fioravante@xxxxxxxxxx>]

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?
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();
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

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