| Message ID | 20230714070931.23476-12-j@getutm.app |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: introduce TPM CRB SysBus device | expand |
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.
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
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 migration path but I realized that I missed the "tpm-crb-cmd" ramblock. It can be added to v3 for this patch. Similar to the logic to have `legacy_regs` we will add a `legacy_cmdmem` memory region that is not registered with the system bus but only exists to migrate the data. Would that work? Also open to better ideas on migrating legacy saved state. If we back out of 4/11 (the split mapping), then the proposed way for working on Apple Silicon would not be available and we would have to go down the path of emulating AArch64 instruction in HVF backend (or decide not to support Apple Silicon).
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'? The expectation is that old VM state / CRB state can be used by the new QEMU and also new QEMU CRB state should be readable by old QEMU. So in a way the old 'secondary' mmaping has to hold the true value of the registers so that the latter works. > migration path but I realized that I missed the "tpm-crb-cmd" > ramblock. It can be added to v3 for this patch. Similar to the logic > to have `legacy_regs` we will add a `legacy_cmdmem` memory region that > is not registered with the system bus but only exists to migrate the > data. Would that work? Also open to better ideas on migrating legacy > saved state. > > If we back out of 4/11 (the split mapping), then the proposed way for > working on Apple Silicon would not be available and we would have to > go down the path of emulating AArch64 instruction in HVF backend (or > decide not to support Apple Silicon).
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
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).
On 7/14/23 14:49, Joelle van Dyne 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 Unfortunately the CRB device is being used by x86 on some distros and the expectation is that this existing device can also downgrade to a previous version of QEMU I would say. I have read people migrating from RHEL 9.x even to RHEL 8.x and the expectation is that this works. Now you are introducing a new device and I think you can leave the existing device with its s->regs alone and have the new device with its mmio regs work a little different just to preserve the QEMU downgrade for x86. > the old code, the only field that gets a dynamic update is > tpmEstablished which we found is never changed. So effectively, .read Correct and that's why you don't need a .read in the new device. > 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 At least not with the .read part as it seems and you have to have the .write part to be able to react to cmd transfers etc. > 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). Thanks, this confirms what I thought. Stefan
On Fri, Jul 14, 2023 at 12:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/14/23 14:49, Joelle van Dyne 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 > > Unfortunately the CRB device is being used by x86 on some distros > and the expectation is that this existing device can also downgrade > to a previous version of QEMU I would say. I have read people migrating > from RHEL 9.x even to RHEL 8.x and the expectation is that this works. But would the migration even work due to other parts of QEMU? The only way you can, say, migrate from QEMU 8.1.0 to 8.0.0 is if every single VMstate has its version_id unchanged. Does QEMU provide that guarantee? I'm fine with changing it but just want to make sure expectations are set correctly. Have you tested a downgrade and found that no other device impeded the process? > > Now you are introducing a new device and I think you can leave > the existing device with its s->regs alone and have the new device > with its mmio regs work a little different just to preserve the QEMU > downgrade for x86. > > > the old code, the only field that gets a dynamic update is > > tpmEstablished which we found is never changed. So effectively, .read > > Correct and that's why you don't need a .read in the new device. > > > 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 > > At least not with the .read part as it seems and you have to have the > .write part to be able to react to cmd transfers etc. > > > 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). > > Thanks, this confirms what I thought. > > Stefan
On 7/14/23 15:44, Joelle van Dyne wrote: > On Fri, Jul 14, 2023 at 12:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/14/23 14:49, Joelle van Dyne 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 >> >> Unfortunately the CRB device is being used by x86 on some distros >> and the expectation is that this existing device can also downgrade >> to a previous version of QEMU I would say. I have read people migrating >> from RHEL 9.x even to RHEL 8.x and the expectation is that this works. > But would the migration even work due to other parts of QEMU? The only > way you can, say, migrate from QEMU 8.1.0 to 8.0.0 is if every single > VMstate has its version_id unchanged. Does QEMU provide that > guarantee? I'm fine with changing it but just want to make sure > expectations are set correctly. Have you tested a downgrade and found > that no other device impeded the process? No I have not done this. The best we can do is that CRB at least is not the reason that is causing such a failure and since we are introducing a new device it need not be the reason, either. Stefan
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
On Fri, 14 Jul 2023 at 20:13, Stefan Berger <stefanb@linux.ibm.com> wrote: > Unfortunately the CRB device is being used by x86 on some distros > and the expectation is that this existing device can also downgrade > to a previous version of QEMU I would say. I have read people migrating > from RHEL 9.x even to RHEL 8.x and the expectation is that this works. If you want both-ways migration compatibility for the change of implementation to use a RAM-backed MR rather than an MMIO MR: * make sure the new RAM-backed memory region is created using a memory_region_init_*_nomigrate() function so that we don't try to migrate the RAM as RAM * keep the regs array in the device struct, and add a comment that it's only used during migration * keep the vmstate entry for the regs array as it is * in the device's vmstate pre_save hook, copy from the backing RAM into the regs array (watch out for endianness issues :-)) * in the vmstate post_load hook, copy from the regs array into the backing RAM -- PMM
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h index 7cdd37335f..7d8f643e98 100644 --- a/hw/tpm/tpm_crb.h +++ b/hw/tpm/tpm_crb.h @@ -70,5 +70,6 @@ enum TPMVersion tpm_crb_get_version(TPMCRBState *s); int tpm_crb_pre_save(TPMCRBState *s); void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr); void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp); +void tpm_crb_restore_regs(TPMCRBState *s, uint32_t *saved_regs); #endif /* TPM_TPM_CRB_H */ diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c index 594696ffb8..be29ca8c28 100644 --- a/hw/tpm/tpm_crb.c +++ b/hw/tpm/tpm_crb.c @@ -40,6 +40,7 @@ struct CRBState { ISADevice parent_obj; TPMCRBState state; + uint32_t legacy_regs[TPM_CRB_R_MAX]; }; typedef struct CRBState CRBState; @@ -67,10 +68,23 @@ static int tpm_crb_isa_pre_save(void *opaque) return tpm_crb_pre_save(&s->state); } +static int tpm_crb_isa_post_load(void *opaque, int version_id) +{ + CRBState *s = opaque; + + if (version_id == 0) { + tpm_crb_restore_regs(&s->state, s->legacy_regs); + } + return 0; +} + static const VMStateDescription vmstate_tpm_crb_isa = { .name = "tpm-crb", + .version_id = 1, .pre_save = tpm_crb_isa_pre_save, + .post_load = tpm_crb_isa_post_load, .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(legacy_regs, CRBState, TPM_CRB_R_MAX), VMSTATE_END_OF_LIST(), } }; diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c index 4ecf064c98..5714ac7fc4 100644 --- a/hw/tpm/tpm_crb_common.c +++ b/hw/tpm/tpm_crb_common.c @@ -224,3 +224,10 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp) tpm_ppi_init_memory(&s->ppi, obj); } } + +void tpm_crb_restore_regs(TPMCRBState *s, uint32_t *saved_regs) +{ + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio); + + memcpy(regs, saved_regs, TPM_CRB_R_MAX); +}
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. Signed-off-by: Joelle van Dyne <j@getutm.app> --- hw/tpm/tpm_crb.h | 1 + hw/tpm/tpm_crb.c | 14 ++++++++++++++ hw/tpm/tpm_crb_common.c | 7 +++++++ 3 files changed, 22 insertions(+)