Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate

[Igor Mammedov]

On Fri, 14 Jul 2023 11:49:03 -0700
Joelle van Dyne <j@getutm.app> wrote:

> On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> >
> >
> > On 7/14/23 14:22, Stefan Berger wrote:  
> > > On 7/14/23 13:04, Joelle van Dyne wrote:  
> > >> On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger <stefanb@linux.ibm.com> 
> > >> wrote:  
> > >>>
> > >>>
> > >>>
> > >>> On 7/14/23 10:05, Stefan Berger wrote:  
> > >>>>
> > >>>>
> > >>>> On 7/14/23 03:09, Joelle van Dyne wrote:  
> > >>>>> When we moved to a single mapping and modified TPM CRB's VMState, it
> > >>>>> broke restoring of VMs that were saved on an older version. This
> > >>>>> change allows those VMs to gracefully migrate to the new memory
> > >>>>> mapping.  
> > >>>>
> > >>>> Thanks. This has to be in 4/11 though.
> > >>>>  
> > >>>
> > >>> After applying the whole series and trying to resume state taken with 
> > >>> current git
> > >>> master I cannot restore it but it leads to this error here. I would 
> > >>> just leave it
> > >>> completely untouched in 4/11.
> > >>>
> > >>> 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock 
> > >>> "tpm-crb-cmd", cannot accept migration
> > >>> 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading 
> > >>> state for instance 0x0 of device 'ram'
> > >>> 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration 
> > >>> failed: Invalid argument
> > >>>
> > >>>      Stefan  
> > >>
> > >> To be clear, you are asking to back out of 4/11? That patch changes
> > >> how the registers are mapped so it's impossible to support the old
> > >> style register mapping. This patch attempts to fix that with a  
> > >
> > > Why can we not keep the old style register mapping as 'secondary 
> > > mapping'?  
> >
> > I think the first goal should be for existing TPM CRB device not to change 
> > anything, they
> > keep their .read and .write behaivor as it.
> >
> > If you need different .read behavior for the sysbus device due to AARCH64 
> > then it may want to use its own MemoryRegionOps.
> >
> > I am fairly sure that you could refactor the core of the existing 
> > tpm_crb_mmio_write() and have it work on s->regs and mmio regs.
> > The former would be used by existing code, the latter for CRB sysbus 
> > calling into this new function from a wrapper.
> >
> >     Stefan  
> 
> I agree that new QEMU should be able to read old QEMU state but vice
> versa is not always true. There's been many changes in the past that
> incremented the vmstate's version_id to indicate that the state format
> has changed. Also, we are not changing the .read behavior because in
> the old code, the only field that gets a dynamic update is
> tpmEstablished which we found is never changed. So effectively, .read
> is just doing a memcpy of the `regs` state. This makes it possible to
> map the page as memory while retaining the same behavior as before.
> (We are changing the code but not the behavior).
> 
> The issue with Windows's buggy tpm.sys driver is that fundamentally it
> cannot work with MemoryRegionOps. The way MMIO is implemented is that
> a hole is left in the guest memory space so when the device registers
> are accessed, the hypervisor traps it and sends it over to QEMU to
> handle. QEMU looks up the address, sees its a valid MMIO mapping, and
> calls into the MemoryRegionOps implementation. When tpm.sys does a LDP
> instruction access to the hole, the information for QEMU to determine
> if it's a valid access is not provided. Other hypervisors like Apple's
> VZ.framework and VMware will read the guest PC, manually decode the
> AArch64 instruction, determine the type of access, read the guest Rn
> registers, does a TLB lookup to determine the physical address, then
> emulate the MMIO. None of this capability currently exists in QEMU's
> ARM64 backend. That's why we decided the easier path is to tell QEMU
> that this mapping is RAM for read purposes and MMIO only for write
> purposes (thankfully Windows does not do a STP or we'd be hosed).

CCing migration and ARM folks for more exposure