On Tue, Sep 06, 2011 at 07:55:48PM -0400, Stefan Berger wrote: > On 09/04/2011 03:32 PM, Michael S. Tsirkin wrote: > >On Thu, Sep 01, 2011 at 09:53:40PM -0400, Stefan Berger wrote: > >>>Generally, what all other devices do is perform validation > >>>as the last step in migration when device state > >>>is restored. On failure, management can decide what to do: > >>>retry migration or restart on source. > >>> > >>>Why is TPM special and needs to be treated differently? > >>> > >>> > >>> > >... > > > >>More detail: Typically one starts out with an empty QCoW2 file > >>created via qemu-img. Once Qemu starts and initializes the > >>libtpms-based TPM, it tries to read existing state from that QCoW2 > >>file. Since there is no state stored in the QCoW2, the TPM will > >>start initializing itself to an initial 'blank' state. > >So it looks like the problem is you access the file when guest isn't > >running. Delaying the initialization until the guest actually starts > >running will solve the problem in a way that is more consistent with > >other qemu devices. > > > I'd agree if there wasn't one more thing: encryption on the data > inside the QCoW2 filesystem > > First: There are two ways to encrypt the data. > > One comes with the QCoW2 type of image and it comes for free. Set > the encryption flag when creating the QCoW2 file and one has to > provide a key to access the QCoW2. I found this mode problematic for > users since it required me to go through the monitor every time I > started the VM. Besides that the key is provided so late that all > devices are already initialized and if the wrong key was provided > the only thing the TPM can do is to go into shutdown mode since > there is state on the QCoW2 but it cannot be decrypted. This also > became problematic when doing migrations with libvirt for example > and one was to have a wrong key/password installed on the target > side -- graceful termination of the migration is impossible. > > So the above drove the implementation of the encryption mode added > in patch 10 in this series. Here the key is provided via command > line and it can be used immediately. So I am reading the state blobs > from the file, decrypt them, create the CRC32 on the plain data and > check against the CRC32 stored in the 'directory'. If it doesn't > match the expected CRC32 either the key was wrong or the state is > corrupted and I can terminate Qemu gracefully. I can also react > appropriately if no key was provided but one is expected and > vice-versa. Also in case of migration this now allows me to > terminate Qemu gracefully so it continues running on the source. > This is an improvement over the situation described above where in > case the target had the wrong key the TPM went into shutdown mode > and the user would be wondering why that is -- the TPM becomes > inaccessible. However, particularly in the case of migration with > shared storage I need to access the QCoW2 file to check whether on > the target I have the right key. And this happens very early during > qemu startup on the target machine. So that's where the file locking > on the block layer comes in. I want to serialize access to the QCoW2 > so I don't read intermediate state on the migration-target host that > can occur if the source is currently writing TPM state data. > > This patch and the command line provided key along with the > encryption mode added in patch 10 in this series add to usability > and help prevent failures. > > Regards, > Stefan
Sure, it's a useful feature of validating the device early. But as I said above, it's useful for other devices besides TPM. However it introduces issues where state changes since guest keeps running (such as what you have described here). I think solving this in a way specific to TPM is a mistake. Passing key on command line does not mean you must use it immediately, this can still be done when guest starts running. Further, the patchset seems to be split in a way that introduces support headaches and makes review harder, not easier. In this case, we will have to support both a broken mode (key supplied later) and a non-broken one (key supplied on init). It would be better to implement a single mode, in a single patch. -- MST
