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 <stef...@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 <stef...@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
