| Message ID | 20230713035232.48406-5-j@getutm.app (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: introduce TPM CRB SysBus device | expand |
On 7/12/23 23:51, Joelle van Dyne wrote: > On Apple Silicon, when Windows performs a LDP on the CRB MMIO space, > the exception is not decoded by hardware and we cannot trap the MMIO > read. This led to the idea from @agraf to use the same mapping type as > ROM devices: namely that reads should be seen as memory type and > writes should trap as MMIO. > > Once that was done, the second memory mapping of the command buffer > region was redundent and was removed. > > A note about the removal of the read trap for `CRB_LOC_STATE`: > The only usage was to return the most up-to-date value for > `tpmEstablished`. However, `tpmEstablished` is only set when a > TPM2_HashStart operation is called which only exists for locality 4. > Indeed, the comment for the write handler of `CRB_LOC_CTRL` makes the > same argument for why it is not calling the backend to reset the > `tpmEstablished` bit. As this bit is unused, we do not need to worry > about updating it for reads. > > Signed-off-by: Joelle van Dyne <j@getutm.app> > --- > hw/tpm/tpm_crb.h | 2 - > hw/tpm/tpm_crb.c | 3 - > hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++-------------------- > 3 files changed, 63 insertions(+), 66 deletions(-) > > diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h > index da3a0cf256..7cdd37335f 100644 > --- a/hw/tpm/tpm_crb.h > +++ b/hw/tpm/tpm_crb.h > @@ -26,9 +26,7 @@ > typedef struct TPMCRBState { > TPMBackend *tpmbe; > TPMBackendCmd cmd; > - uint32_t regs[TPM_CRB_R_MAX]; > MemoryRegion mmio; > - MemoryRegion cmdmem; > > size_t be_buffer_size; > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c > index 598c3e0161..07c6868d8d 100644 > --- a/hw/tpm/tpm_crb.c > +++ b/hw/tpm/tpm_crb.c > @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = { > .name = "tpm-crb", > .pre_save = tpm_crb_none_pre_save, > .fields = (VMStateField[]) { > - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX), This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded. 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan
On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/12/23 23:51, Joelle van Dyne wrote: > > On Apple Silicon, when Windows performs a LDP on the CRB MMIO space, > > the exception is not decoded by hardware and we cannot trap the MMIO > > read. This led to the idea from @agraf to use the same mapping type as > > ROM devices: namely that reads should be seen as memory type and > > writes should trap as MMIO. These are hardware registers, right? Windows shouldn't really be doing LDP to those if it expects to be able to run in a VM... > > Once that was done, the second memory mapping of the command buffer > > region was redundent and was removed. > > > > A note about the removal of the read trap for `CRB_LOC_STATE`: > > The only usage was to return the most up-to-date value for > > `tpmEstablished`. However, `tpmEstablished` is only set when a > > TPM2_HashStart operation is called which only exists for locality 4. > > Indeed, the comment for the write handler of `CRB_LOC_CTRL` makes the > > same argument for why it is not calling the backend to reset the > > `tpmEstablished` bit. As this bit is unused, we do not need to worry > > about updating it for reads. > > > > Signed-off-by: Joelle van Dyne <j@getutm.app> > > --- > > hw/tpm/tpm_crb.h | 2 - > > hw/tpm/tpm_crb.c | 3 - > > hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++-------------------- > > 3 files changed, 63 insertions(+), 66 deletions(-) > > > > diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h > > index da3a0cf256..7cdd37335f 100644 > > --- a/hw/tpm/tpm_crb.h > > +++ b/hw/tpm/tpm_crb.h > > @@ -26,9 +26,7 @@ > > typedef struct TPMCRBState { > > TPMBackend *tpmbe; > > TPMBackendCmd cmd; > > - uint32_t regs[TPM_CRB_R_MAX]; > > MemoryRegion mmio; > > - MemoryRegion cmdmem; > > > > size_t be_buffer_size; > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c > > index 598c3e0161..07c6868d8d 100644 > > --- a/hw/tpm/tpm_crb.c > > +++ b/hw/tpm/tpm_crb.c > > @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = { > > .name = "tpm-crb", > > .pre_save = tpm_crb_none_pre_save, > > .fields = (VMStateField[]) { > > - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX), > > This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded. > > 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration > 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' > 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument More generally, for migration compatibility in the other direction you need to use memory_region_init_rom_device_nomigrate() and make sure you keep migrating the data via this, not via the MemoryRegion. I'm not a super-fan of hacking around the fact that LDP to hardware registers isn't supported in specific device models, though... thanks -- PMM
On 7/13/23 10:50, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/12/23 23:51, Joelle van Dyne wrote: >>> On Apple Silicon, when Windows performs a LDP on the CRB MMIO space, >>> the exception is not decoded by hardware and we cannot trap the MMIO >>> read. This led to the idea from @agraf to use the same mapping type as >>> ROM devices: namely that reads should be seen as memory type and >>> writes should trap as MMIO. >>> +++ b/hw/tpm/tpm_crb.c >>> @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = { >>> .name = "tpm-crb", >>> .pre_save = tpm_crb_none_pre_save, >>> .fields = (VMStateField[]) { >>> - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX), >> >> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded. >> >> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration >> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' >> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument > > More generally, for migration compatibility in the other > direction you need to use memory_region_init_rom_device_nomigrate() > and make sure you keep migrating the data via this, not > via the MemoryRegion. > > I'm not a super-fan of hacking around the fact that LDP > to hardware registers isn't supported in specific device > models, though... What does this mean for this effort here? Stefan > > thanks > -- PMM
On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/13/23 10:50, Peter Maydell wrote: > > On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 7/12/23 23:51, Joelle van Dyne wrote: > >>> On Apple Silicon, when Windows performs a LDP on the CRB MMIO space, > >>> the exception is not decoded by hardware and we cannot trap the MMIO > >>> read. This led to the idea from @agraf to use the same mapping type as > >>> ROM devices: namely that reads should be seen as memory type and > >>> writes should trap as MMIO. > > >>> +++ b/hw/tpm/tpm_crb.c > >>> @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = { > >>> .name = "tpm-crb", > >>> .pre_save = tpm_crb_none_pre_save, > >>> .fields = (VMStateField[]) { > >>> - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX), > >> > >> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded. > >> > >> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration > >> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' > >> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument > > > > More generally, for migration compatibility in the other > > direction you need to use memory_region_init_rom_device_nomigrate() > > and make sure you keep migrating the data via this, not > > via the MemoryRegion. > > > > I'm not a super-fan of hacking around the fact that LDP > > to hardware registers isn't supported in specific device > > models, though... > > What does this mean for this effort here? Usually we say "fix the guest to not try to access hardware registers with silly load/store instruction types". The other option would be "put in a large amount of effort to support emulating those instructions in QEMU userspace when KVM/HVF/etc trap and punt them to us". For the last decade or so we have taken the first of these approaches :-) thanks -- PMM
On 7/13/23 11:34, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/13/23 10:50, Peter Maydell wrote: >>> On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 7/12/23 23:51, Joelle van Dyne wrote: >>>>> On Apple Silicon, when Windows performs a LDP on the CRB MMIO space, >>>>> the exception is not decoded by hardware and we cannot trap the MMIO >>>>> read. This led to the idea from @agraf to use the same mapping type as >>>>> ROM devices: namely that reads should be seen as memory type and >>>>> writes should trap as MMIO. >> >>>>> +++ b/hw/tpm/tpm_crb.c >>>>> @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = { >>>>> .name = "tpm-crb", >>>>> .pre_save = tpm_crb_none_pre_save, >>>>> .fields = (VMStateField[]) { >>>>> - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX), >>>> >>>> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded. >>>> >>>> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration >>>> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' >>>> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument >>> >>> More generally, for migration compatibility in the other >>> direction you need to use memory_region_init_rom_device_nomigrate() >>> and make sure you keep migrating the data via this, not >>> via the MemoryRegion. >>> >>> I'm not a super-fan of hacking around the fact that LDP >>> to hardware registers isn't supported in specific device >>> models, though... >> >> What does this mean for this effort here? > > Usually we say "fix the guest to not try to access hardware > registers with silly load/store instruction types". The other > option would be "put in a large amount of effort to support > emulating those instructions in QEMU userspace when KVM/HVF/etc > trap and punt them to us". For the last decade or so we have > taken the first of these approaches :-) Is Microsoft likely to react to use telling them "fix the guest"? Stefan > > thanks > -- PMM
On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote: > On 7/13/23 11:34, Peter Maydell wrote: > > On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote: > >> On 7/13/23 10:50, Peter Maydell wrote: > >>> I'm not a super-fan of hacking around the fact that LDP > >>> to hardware registers isn't supported in specific device > >>> models, though... > >> > >> What does this mean for this effort here? > > > > Usually we say "fix the guest to not try to access hardware > > registers with silly load/store instruction types". The other > > option would be "put in a large amount of effort to support > > emulating those instructions in QEMU userspace when KVM/HVF/etc > > trap and punt them to us". For the last decade or so we have > > taken the first of these approaches :-) > > Is Microsoft likely to react to use telling them "fix the guest"? They have on occasion in the past, yes. The other outstanding question here is if this TPM device should be a sysbus one at all (i.e. not i2c), which might render this part moot. thanks -- PMM
On 7/13/23 11:55, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote: >> On 7/13/23 11:34, Peter Maydell wrote: >>> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> On 7/13/23 10:50, Peter Maydell wrote: >>>>> I'm not a super-fan of hacking around the fact that LDP >>>>> to hardware registers isn't supported in specific device >>>>> models, though... >>>> >>>> What does this mean for this effort here? >>> >>> Usually we say "fix the guest to not try to access hardware >>> registers with silly load/store instruction types". The other >>> option would be "put in a large amount of effort to support >>> emulating those instructions in QEMU userspace when KVM/HVF/etc >>> trap and punt them to us". For the last decade or so we have >>> taken the first of these approaches :-) >> >> Is Microsoft likely to react to use telling them "fix the guest"? > > They have on occasion in the past, yes. > > The other outstanding question here is if this TPM device > should be a sysbus one at all (i.e. not i2c), which might > render this part moot. Does the aarch64 virt VM support an i2c bus? Would it support the aspeed i2c bus? Does Windows then accept this i2c bus? Maybe the faster answer comes via this device that Joelle presumably has working on AARCH64 Windows. Stefan > > thanks > -- PMM
On Thu, 13 Jul 2023 at 17:54, Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/13/23 11:55, Peter Maydell wrote: > > On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote: > >> On 7/13/23 11:34, Peter Maydell wrote: > >>> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>> On 7/13/23 10:50, Peter Maydell wrote: > >>>>> I'm not a super-fan of hacking around the fact that LDP > >>>>> to hardware registers isn't supported in specific device > >>>>> models, though... > >>>> > >>>> What does this mean for this effort here? > >>> > >>> Usually we say "fix the guest to not try to access hardware > >>> registers with silly load/store instruction types". The other > >>> option would be "put in a large amount of effort to support > >>> emulating those instructions in QEMU userspace when KVM/HVF/etc > >>> trap and punt them to us". For the last decade or so we have > >>> taken the first of these approaches :-) > >> > >> Is Microsoft likely to react to use telling them "fix the guest"? > > > > They have on occasion in the past, yes. > > > > The other outstanding question here is if this TPM device > > should be a sysbus one at all (i.e. not i2c), which might > > render this part moot. > > Does the aarch64 virt VM support an i2c bus? Would it support the aspeed i2c bus? Does Windows then accept this i2c bus? Maybe the faster answer comes via this device that Joelle presumably has working on AARCH64 Windows. The aim is not "get Windows booting as fast as possible", though. It's to end up with a QEMU virt board that (a) is maintainable (b) is reasonably congruent with what real hardware does (c) works in a way that will also work with what other guest OSes are expecting. I don't want to accept changes to the virt board that are hard to live with in future, because changing virt in non-backward compatible ways is painful. thanks -- PMM
On 7/13/23 13:07, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 17:54, Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/13/23 11:55, Peter Maydell wrote: >>> On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> On 7/13/23 11:34, Peter Maydell wrote: >>>>> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>>> On 7/13/23 10:50, Peter Maydell wrote: >>>>>>> I'm not a super-fan of hacking around the fact that LDP >>>>>>> to hardware registers isn't supported in specific device >>>>>>> models, though... >>>>>> >>>>>> What does this mean for this effort here? >>>>> >>>>> Usually we say "fix the guest to not try to access hardware >>>>> registers with silly load/store instruction types". The other >>>>> option would be "put in a large amount of effort to support >>>>> emulating those instructions in QEMU userspace when KVM/HVF/etc >>>>> trap and punt them to us". For the last decade or so we have >>>>> taken the first of these approaches :-) >>>> >>>> Is Microsoft likely to react to use telling them "fix the guest"? >>> >>> They have on occasion in the past, yes. >>> >>> The other outstanding question here is if this TPM device >>> should be a sysbus one at all (i.e. not i2c), which might >>> render this part moot. >> >> Does the aarch64 virt VM support an i2c bus? Would it support the aspeed i2c bus? Does Windows then accept this i2c bus? Maybe the faster answer comes via this device that Joelle presumably has working on AARCH64 Windows. > > The aim is not "get Windows booting as fast as possible", though. > It's to end up with a QEMU virt board that (a) is maintainable > (b) is reasonably congruent with what real hardware does > (c) works in a way that will also work with what other > guest OSes are expecting. > > I don't want to accept changes to the virt board that are > hard to live with in future, because changing virt in > non-backward compatible ways is painful. > I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. It seems sysbus is already supported there so ... we may have a 'match'? dev = qdev_new("arm-gicv2m"); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_V2M].base); qdev_prop_set_uint32(dev, "base-spi", irq); qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Stefan > thanks > -- PMM
On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> wrote: > I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. > > It seems sysbus is already supported there so ... we may have a 'match'? You can use sysbus devices anywhere -- they're just "this is a memory mapped device". The question is whether we should, or whether an i2c controller is more like what the real world uses (and if so, what i2c controller). -- PMM
On 7/13/23 13:18, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> wrote: >> I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. >> >> It seems sysbus is already supported there so ... we may have a 'match'? > > You can use sysbus devices anywhere -- they're just 'anywhere' also includes aarch64 virt board I suppose. > "this is a memory mapped device". The question is whether > we should, or whether an i2c controller is more like > what the real world uses (and if so, what i2c controller). > > I don't want to accept changes to the virt board that are > hard to live with in future, because changing virt in > non-backward compatible ways is painful. Once we have the CRB sysbus device we would keep it around forever and it seems to - not require any changes to the virt board (iiuc) since sysbus is already being used - works already with Windows and probably also Linux Stefan > -- PMM
On Thu, 13 Jul 2023 at 19:43, Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/13/23 13:18, Peter Maydell wrote: > > On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> wrote: > >> I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. > >> > >> It seems sysbus is already supported there so ... we may have a 'match'? > > > > You can use sysbus devices anywhere -- they're just > > 'anywhere' also includes aarch64 virt board I suppose. Yes. Literally any machine can have memory mapped devices. > > "this is a memory mapped device". The question is whether > > we should, or whether an i2c controller is more like > > what the real world uses (and if so, what i2c controller). > > > > > I don't want to accept changes to the virt board that are > > hard to live with in future, because changing virt in > > non-backward compatible ways is painful. > > Once we have the CRB sysbus device we would keep it around forever and it seems to > - not require any changes to the virt board (iiuc) since sysbus is already being used > - works already with Windows and probably also Linux "Add a sysbus device to the virt board" is the kind of change I mean -- once you do that it's hard to take it out again, and if we decide in 6 months time that actually i2c would be the better option then we end up with two different ways to do the same thing and trying to deprecate the other one is a pain. -- PMM
On 7/14/23 06:05, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 19:43, Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/13/23 13:18, Peter Maydell wrote: >>> On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. >>>> >>>> It seems sysbus is already supported there so ... we may have a 'match'? >>> >>> You can use sysbus devices anywhere -- they're just >> >> 'anywhere' also includes aarch64 virt board I suppose. > > Yes. Literally any machine can have memory mapped devices. > >>> "this is a memory mapped device". The question is whether >>> we should, or whether an i2c controller is more like >>> what the real world uses (and if so, what i2c controller). >>> >> >>> I don't want to accept changes to the virt board that are >>> hard to live with in future, because changing virt in >>> non-backward compatible ways is painful. >> >> Once we have the CRB sysbus device we would keep it around forever and it seems to >> - not require any changes to the virt board (iiuc) since sysbus is already being used >> - works already with Windows and probably also Linux > > "Add a sysbus device to the virt board" is the kind of > change I mean -- once you do that it's hard to take it > out again, and if we decide in 6 months time that actually > i2c would be the better option then we end up with two > different ways to do the same thing and trying to > deprecate the other one is a pain. At least CRB is a standard interface and from this perspective we are fine. I am not sure what would drive the introduction of the i2c bus in 6 months. I suppose one could then still use sysbus CRB device or the i2c device. The sysbus CRB device should still work then. Anyway, I think we should continue with this series. Stefan > > -- PMM
On Fri, Jul 14, 2023 at 4:57 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/14/23 06:05, Peter Maydell wrote: > > On Thu, 13 Jul 2023 at 19:43, Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 7/13/23 13:18, Peter Maydell wrote: > >>> On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>> I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. > >>>> > >>>> It seems sysbus is already supported there so ... we may have a 'match'? > >>> > >>> You can use sysbus devices anywhere -- they're just > >> > >> 'anywhere' also includes aarch64 virt board I suppose. > > > > Yes. Literally any machine can have memory mapped devices. > > > >>> "this is a memory mapped device". The question is whether > >>> we should, or whether an i2c controller is more like > >>> what the real world uses (and if so, what i2c controller). > >>> > >> > >>> I don't want to accept changes to the virt board that are > >>> hard to live with in future, because changing virt in > >>> non-backward compatible ways is painful. > >> > >> Once we have the CRB sysbus device we would keep it around forever and it seems to > >> - not require any changes to the virt board (iiuc) since sysbus is already being used > >> - works already with Windows and probably also Linux > > > > "Add a sysbus device to the virt board" is the kind of > > change I mean -- once you do that it's hard to take it > > out again, and if we decide in 6 months time that actually > > i2c would be the better option then we end up with two > > different ways to do the same thing and trying to > > deprecate the other one is a pain. > > > At least CRB is a standard interface and from this perspective we are fine. I am not sure what would drive the introduction of the i2c bus in 6 months. I suppose one could then still use sysbus CRB device or the i2c device. The sysbus CRB device should still work then. Anyway, I think we should continue with this series. > > Stefan > > > > > -- PMM FWIW the Windows 11 tpm.sys driver does not support the I2C interface. The driver only recognizes ACPI devices and the case for Start Method = 12 (FIFO Interface over I2C bus) goes to an error handler.
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h index da3a0cf256..7cdd37335f 100644 --- a/hw/tpm/tpm_crb.h +++ b/hw/tpm/tpm_crb.h @@ -26,9 +26,7 @@ typedef struct TPMCRBState { TPMBackend *tpmbe; TPMBackendCmd cmd; - uint32_t regs[TPM_CRB_R_MAX]; MemoryRegion mmio; - MemoryRegion cmdmem; size_t be_buffer_size; diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c index 598c3e0161..07c6868d8d 100644 --- a/hw/tpm/tpm_crb.c +++ b/hw/tpm/tpm_crb.c @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = { .name = "tpm-crb", .pre_save = tpm_crb_none_pre_save, .fields = (VMStateField[]) { - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX), VMSTATE_END_OF_LIST(), } }; @@ -103,8 +102,6 @@ static void tpm_crb_none_realize(DeviceState *dev, Error **errp) memory_region_add_subregion(get_system_memory(), TPM_CRB_ADDR_BASE, &s->state.mmio); - memory_region_add_subregion(get_system_memory(), - TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->state.cmdmem); if (s->state.ppi_enabled) { memory_region_add_subregion(get_system_memory(), diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c index e56e910670..f3e40095e3 100644 --- a/hw/tpm/tpm_crb_common.c +++ b/hw/tpm/tpm_crb_common.c @@ -33,31 +33,12 @@ #include "qom/object.h" #include "tpm_crb.h" -static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr, - unsigned size) +static uint8_t tpm_crb_get_active_locty(TPMCRBState *s, uint32_t *regs) { - TPMCRBState *s = opaque; - void *regs = (void *)&s->regs + (addr & ~3); - unsigned offset = addr & 3; - uint32_t val = *(uint32_t *)regs >> (8 * offset); - - switch (addr) { - case A_CRB_LOC_STATE: - val |= !tpm_backend_get_tpm_established_flag(s->tpmbe); - break; - } - - trace_tpm_crb_mmio_read(addr, size, val); - - return val; -} - -static uint8_t tpm_crb_get_active_locty(TPMCRBState *s) -{ - if (!ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, locAssigned)) { + if (!ARRAY_FIELD_EX32(regs, CRB_LOC_STATE, locAssigned)) { return TPM_CRB_NO_LOCALITY; } - return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality); + return ARRAY_FIELD_EX32(regs, CRB_LOC_STATE, activeLocality); } static void tpm_crb_mmio_write(void *opaque, hwaddr addr, @@ -65,35 +46,47 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr, { TPMCRBState *s = opaque; uint8_t locty = addr >> 12; + uint32_t *regs; + void *mem; trace_tpm_crb_mmio_write(addr, size, val); + regs = memory_region_get_ram_ptr(&s->mmio); + mem = ®s[R_CRB_DATA_BUFFER]; + assert(regs); + + if (addr >= A_CRB_DATA_BUFFER) { + assert(addr + size <= TPM_CRB_ADDR_SIZE); + assert(size <= sizeof(val)); + memcpy(mem + addr - A_CRB_DATA_BUFFER, &val, size); + memory_region_set_dirty(&s->mmio, addr, size); + return; + } switch (addr) { case A_CRB_CTRL_REQ: switch (val) { case CRB_CTRL_REQ_CMD_READY: - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS, tpmIdle, 0); break; case CRB_CTRL_REQ_GO_IDLE: - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS, tpmIdle, 1); break; } break; case A_CRB_CTRL_CANCEL: if (val == CRB_CANCEL_INVOKE && - s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) { + regs[R_CRB_CTRL_START] & CRB_START_INVOKE) { tpm_backend_cancel_cmd(s->tpmbe); } break; case A_CRB_CTRL_START: if (val == CRB_START_INVOKE && - !(s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) && - tpm_crb_get_active_locty(s) == locty) { - void *mem = memory_region_get_ram_ptr(&s->cmdmem); + !(regs[R_CRB_CTRL_START] & CRB_START_INVOKE) && + tpm_crb_get_active_locty(s, regs) == locty) { - s->regs[R_CRB_CTRL_START] |= CRB_START_INVOKE; + regs[R_CRB_CTRL_START] |= CRB_START_INVOKE; s->cmd = (TPMBackendCmd) { .in = mem, .in_len = MIN(tpm_cmd_get_size(mem), s->be_buffer_size), @@ -110,26 +103,27 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr, /* not loc 3 or 4 */ break; case CRB_LOC_CTRL_RELINQUISH: - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE, + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE, locAssigned, 0); - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS, + ARRAY_FIELD_DP32(regs, CRB_LOC_STS, Granted, 0); break; case CRB_LOC_CTRL_REQUEST_ACCESS: - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS, + ARRAY_FIELD_DP32(regs, CRB_LOC_STS, Granted, 1); - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS, + ARRAY_FIELD_DP32(regs, CRB_LOC_STS, beenSeized, 0); - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE, + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE, locAssigned, 1); break; } break; } + + memory_region_set_dirty(&s->mmio, 0, A_CRB_DATA_BUFFER); } const MemoryRegionOps tpm_crb_memory_ops = { - .read = tpm_crb_mmio_read, .write = tpm_crb_mmio_write, .endianness = DEVICE_LITTLE_ENDIAN, .valid = { @@ -140,12 +134,16 @@ const MemoryRegionOps tpm_crb_memory_ops = { void tpm_crb_request_completed(TPMCRBState *s, int ret) { - s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE; + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio); + + assert(regs); + regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE; if (ret != 0) { - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS, tpmSts, 1); /* fatal error */ } - memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE); + + memory_region_set_dirty(&s->mmio, 0, TPM_CRB_ADDR_SIZE); } enum TPMVersion tpm_crb_get_version(TPMCRBState *s) @@ -162,45 +160,48 @@ int tpm_crb_pre_save(TPMCRBState *s) void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr) { + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio); + + assert(regs); if (s->ppi_enabled) { tpm_ppi_reset(&s->ppi); } tpm_backend_reset(s->tpmbe); - memset(s->regs, 0, sizeof(s->regs)); + memset(regs, 0, TPM_CRB_ADDR_SIZE); - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE, + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE, tpmRegValidSts, 1); - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS, tpmIdle, 1); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, InterfaceVersion, CRB_INTF_VERSION_CRB); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, CapCRB, CRB_INTF_CAP_CRB_SUPPORTED); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID, RID, 0b0000); - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2, + ARRAY_FIELD_DP32(regs, CRB_INTF_ID2, VID, PCI_VENDOR_ID_IBM); baseaddr += A_CRB_DATA_BUFFER; - s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE; - s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr; - s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32); - s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE; - s->regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr; - s->regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32); + regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE; + regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr; + regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32); + regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE; + regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr; + regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32); s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe), CRB_CTRL_CMD_SIZE); @@ -208,14 +209,15 @@ void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr) if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) { exit(1); } + + memory_region_rom_device_set_romd(&s->mmio, true); + memory_region_set_dirty(&s->mmio, 0, TPM_CRB_ADDR_SIZE); } void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp) { - memory_region_init_io(&s->mmio, obj, &tpm_crb_memory_ops, s, - "tpm-crb-mmio", sizeof(s->regs)); - memory_region_init_ram(&s->cmdmem, obj, - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); + memory_region_init_rom_device(&s->mmio, obj, &tpm_crb_memory_ops, s, + "tpm-crb-mmio", TPM_CRB_ADDR_SIZE, errp); if (s->ppi_enabled) { tpm_ppi_init_memory(&s->ppi, obj); }
On Apple Silicon, when Windows performs a LDP on the CRB MMIO space, the exception is not decoded by hardware and we cannot trap the MMIO read. This led to the idea from @agraf to use the same mapping type as ROM devices: namely that reads should be seen as memory type and writes should trap as MMIO. Once that was done, the second memory mapping of the command buffer region was redundent and was removed. A note about the removal of the read trap for `CRB_LOC_STATE`: The only usage was to return the most up-to-date value for `tpmEstablished`. However, `tpmEstablished` is only set when a TPM2_HashStart operation is called which only exists for locality 4. Indeed, the comment for the write handler of `CRB_LOC_CTRL` makes the same argument for why it is not calling the backend to reset the `tpmEstablished` bit. As this bit is unused, we do not need to worry about updating it for reads. Signed-off-by: Joelle van Dyne <j@getutm.app> --- hw/tpm/tpm_crb.h | 2 - hw/tpm/tpm_crb.c | 3 - hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 66 deletions(-)