From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Fioravante Subject: Re: [PATCH 2/4] stubdom/vtpm: Add reconfiguration support Date: Thu, 29 Nov 2012 14:47:17 -0500 Message-ID: <50B7BBC5.1020203@jhuapl.edu> References: <50B4D060.9070403@jhuapl.edu> <1354029286-17652-1-git-send-email-dgdegra@tycho.nsa.gov> <1354029286-17652-3-git-send-email-dgdegra@tycho.nsa.gov> <50B7AF3A.3090009@jhuapl.edu> <50B7B778.5020806@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4262235515863216802==" Return-path: In-Reply-To: <50B7B778.5020806@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel De Graaf Cc: "Ian.Campbell@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org This is a cryptographically signed message in MIME format. --===============4262235515863216802== Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha1; boundary="------------ms070709070307060804090102" This is a cryptographically signed message in MIME format. --------------ms070709070307060804090102 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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 d= rivers, pv_grub and the linux guest. The Xenbus Reconfigure states are ne= w to me. Is this intended behavior in line with the original purpose of t= he 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 automatical= ly 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 i= t is > closed: it removes xenstore entries and frees the data structures, and = so > requires the xenstore directory to be re-created. I originally wanted t= o keep > the shutdown semantics the same (to preserve the vtpm auto-shutdown), a= lthough > 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 van= ishes. A cursory look at blkback in linux and it looks like the backend driver=20 doesn't tear down the xenstore entries at all. I think its supposed to=20 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=20 maintain enough state so that the device can reconnect later. > =20 >> 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 >>> --- >>> 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/inclu= de/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 =3D 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_tpmfron= t(). >>> { >>> 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 =3D=3D XenbusStateConnected) { >>> - dev->state =3D XenbusStateClosing; >>> - //FIXME: Transaction for this? >>> + dev->state =3D for_reconfig ? XenbusStateReconfiguring : Xenbu= sStateClosing; >>> /* Tell backend we are closing */ >>> if((err =3D 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 =3D XenbusStateClosed; >>> + dev->state =3D for_reconfig ? XenbusStateInitialising : Xenbus= StateClosed; >>> if((err =3D xenbus_printf(XBT_NIL, dev->nodename, "state", "= %u", (unsigned int) dev->state))) { >>> TPMFRONT_ERR("Unable to write to %s, error was %s", dev->node= name, err); >>> free(err); >>> } >>> /* Wait for the backend to close and unmap shared pages, i= gnore 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(); >>> =20 >> > --------------ms070709070307060804090102 Content-Type: application/pkcs7-signature; name="smime.p7s" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="smime.p7s" Content-Description: S/MIME Cryptographic Signature MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIIDyjCC A8YwggMvoAMCAQICBD/xyf0wDQYJKoZIhvcNAQEFBQAwLzELMAkGA1UEBhMCVVMxDzANBgNV BAoTBkpIVUFQTDEPMA0GA1UECxMGQklTRENBMB4XDTEwMDYxMTE4MjIwNloXDTEzMDYxMTE4 NTIwNlowZjELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkpIVUFQTDEPMA0GA1UECxMGUGVvcGxl MTUwFgYDVQQLEw9WUE5Hcm91cC1CSVNEQ0EwGwYDVQQDExRNYXR0aGV3IEUgRmlvcmF2YW50 ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAnpbwVSP6o1Nb5lcW7dd3yTo9iBJdi7qz 4nANOMFPK7JOy5npKN1iiousl28U/scUJES55gPwAWYJK3uVyQAsA4adgDKi5DoD1UHDQEwp bY7iHLJeq0NPr4BqYNqnCFPbE6HC8zSJrr4qKn+gVUQT39SIFqdiIPJwZL8FYTRQ/zsCAwEA AaOCAbYwggGyMAsGA1UdDwQEAwIHgDArBgNVHRAEJDAigA8yMDEwMDYxMTE4MjIwNlqBDzIw MTIwNzE3MjI1MjA2WjAbBg0rBgEEAbMlCwMBAQEBBAoWCGZpb3JhbWUxMBsGDSsGAQQBsyUL AwEBAQIEChIIMDAxMDQyNjEwWAYJYIZIAYb6ax4BBEsMSVRoZSBwcml2YXRlIGtleSBjb3Jy ZXNwb25kaW5nIHRvIHRoaXMgY2VydGlmaWNhdGUgbWF5IGhhdmUgYmVlbiBleHBvcnRlZC4w KAYDVR0RBCEwH4EdTWF0dGhldy5GaW9yYXZhbnRlQGpodWFwbC5lZHUwUgYDVR0fBEswSTBH oEWgQ6RBMD8xCzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZKSFVBUEwxDzANBgNVBAsTBkJJU0RD QTEOMAwGA1UEAxMFQ1JMNTYwHwYDVR0jBBgwFoAUCDUpmxH52EU2CyWmF2EJMB1yqeswHQYD VR0OBBYEFO6LYxg6r9wHZ+zdQtBHn1dZ/YTNMAkGA1UdEwQCMAAwGQYJKoZIhvZ9B0EABAww ChsEVjcuMQMCBLAwDQYJKoZIhvcNAQEFBQADgYEAJO9HQh4YNChVLzuZqK5ARJARD8JoujGZ fdo75quvg2jXFQe2sEjvLnxJZgm/pv8fdZakq48CWwjYHKuvIp7sDjTEsQfo+y7SpN/N2NvJ WU5SqfK1VgYtNLRRoGJUB5Q1aZ+Dg95g3kqpyfpUMISJL8IKVLtJVfN4fggFVUYZ9wwxggGr MIIBpwIBATA3MC8xCzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZKSFVBUEwxDzANBgNVBAsTBkJJ U0RDQQIEP/HJ/TAJBgUrDgMCGgUAoIHLMBgGCSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJ KoZIhvcNAQkFMQ8XDTEyMTEyOTE5NDcxN1owIwYJKoZIhvcNAQkEMRYEFE/gZQStCCg3VYiu 7xDXQfrUK7iEMGwGCSqGSIb3DQEJDzFfMF0wCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBAjAK BggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZIhvcNAwICAUAwBwYFKw4DAgcwDQYI KoZIhvcNAwICASgwDQYJKoZIhvcNAQEBBQAEgYCPxyYSTt+deJP3HQ9pUQb8Z4uWdL4Hc48l fu4W/UHE4CXbGl15KYpOiwkMUmAPlPTAiHm1tgaz/ak8ScE1LIEbxBDx47NiaHfnZCOmtBBV 2tmWpcJH9MZ+R7u8OsxGp0iw9cYy0kari/qS6ZUbqxGzHu+5co+WScggNiwBpXOw1QAAAAAA AA== --------------ms070709070307060804090102-- --===============4262235515863216802== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4262235515863216802==--