[v2,05/11] tpm_crb: use the ISA bus

Since this device is gated to only build for targets with the PC
configuration, we should use the ISA bus like with TPM TIS.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 hw/tpm/tpm_crb.c | 52 ++++++++++++++++++++++++------------------------
 hw/tpm/Kconfig   |  2 +-
 2 files changed, 27 insertions(+), 27 deletions(-)

On Fri, 14 Jul 2023 00:09:21 -0700
Joelle van Dyne <j@getutm.app> wrote:

> Since this device is gated to only build for targets with the PC
> configuration, we should use the ISA bus like with TPM TIS.

does it affect migration in any way?
From guest pov it looks like there a new ISA device will appear
and then if you do ping pong migration between old - new QEMU will really it work?

If it will, then commit message here shall describe why it safe and why it works


> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>  hw/tpm/tpm_crb.c | 52 ++++++++++++++++++++++++------------------------
>  hw/tpm/Kconfig   |  2 +-
>  2 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 07c6868d8d..6144081d30 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -22,6 +22,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/pci/pci_ids.h"
>  #include "hw/acpi/tpm.h"
> +#include "hw/isa/isa.h"
>  #include "migration/vmstate.h"
>  #include "sysemu/tpm_backend.h"
>  #include "sysemu/tpm_util.h"
> @@ -34,7 +35,7 @@
>  #include "tpm_crb.h"
>  
>  struct CRBState {
> -    DeviceState parent_obj;
> +    ISADevice parent_obj;
>  
>      TPMCRBState state;
>  };
> @@ -43,49 +44,49 @@ typedef struct CRBState CRBState;
>  DECLARE_INSTANCE_CHECKER(CRBState, CRB,
>                           TYPE_TPM_CRB)
>  
> -static void tpm_crb_none_request_completed(TPMIf *ti, int ret)
> +static void tpm_crb_isa_request_completed(TPMIf *ti, int ret)
>  {
>      CRBState *s = CRB(ti);
>  
>      tpm_crb_request_completed(&s->state, ret);
>  }
>  
> -static enum TPMVersion tpm_crb_none_get_version(TPMIf *ti)
> +static enum TPMVersion tpm_crb_isa_get_version(TPMIf *ti)
>  {
>      CRBState *s = CRB(ti);
>  
>      return tpm_crb_get_version(&s->state);
>  }
>  
> -static int tpm_crb_none_pre_save(void *opaque)
> +static int tpm_crb_isa_pre_save(void *opaque)
>  {
>      CRBState *s = opaque;
>  
>      return tpm_crb_pre_save(&s->state);
>  }
>  
> -static const VMStateDescription vmstate_tpm_crb_none = {
> +static const VMStateDescription vmstate_tpm_crb_isa = {
>      .name = "tpm-crb",
> -    .pre_save = tpm_crb_none_pre_save,
> +    .pre_save = tpm_crb_isa_pre_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_END_OF_LIST(),
>      }
>  };
>  
> -static Property tpm_crb_none_properties[] = {
> +static Property tpm_crb_isa_properties[] = {
>      DEFINE_PROP_TPMBE("tpmdev", CRBState, state.tpmbe),
>      DEFINE_PROP_BOOL("ppi", CRBState, state.ppi_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static void tpm_crb_none_reset(void *dev)
> +static void tpm_crb_isa_reset(void *dev)
>  {
>      CRBState *s = CRB(dev);
>  
>      return tpm_crb_reset(&s->state, TPM_CRB_ADDR_BASE);
>  }
>  
> -static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
> +static void tpm_crb_isa_realize(DeviceState *dev, Error **errp)
>  {
>      CRBState *s = CRB(dev);
>  
> @@ -100,52 +101,51 @@ static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
>  
>      tpm_crb_init_memory(OBJECT(s), &s->state, errp);
>  
> -    memory_region_add_subregion(get_system_memory(),
> +    memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>          TPM_CRB_ADDR_BASE, &s->state.mmio);
>  
>      if (s->state.ppi_enabled) {
> -        memory_region_add_subregion(get_system_memory(),
> +        memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>              TPM_PPI_ADDR_BASE, &s->state.ppi.ram);
>      }
>  
>      if (xen_enabled()) {
> -        tpm_crb_none_reset(dev);
> +        tpm_crb_isa_reset(dev);
>      } else {
> -        qemu_register_reset(tpm_crb_none_reset, dev);
> +        qemu_register_reset(tpm_crb_isa_reset, dev);
>      }
>  }
>  
> -static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
> +static void tpm_crb_isa_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      TPMIfClass *tc = TPM_IF_CLASS(klass);
>  
> -    dc->realize = tpm_crb_none_realize;
> -    device_class_set_props(dc, tpm_crb_none_properties);
> -    dc->vmsd  = &vmstate_tpm_crb_none;
> +    dc->realize = tpm_crb_isa_realize;
> +    device_class_set_props(dc, tpm_crb_isa_properties);
> +    dc->vmsd  = &vmstate_tpm_crb_isa;
>      dc->user_creatable = true;
>      tc->model = TPM_MODEL_TPM_CRB;
> -    tc->get_version = tpm_crb_none_get_version;
> -    tc->request_completed = tpm_crb_none_request_completed;
> +    tc->get_version = tpm_crb_isa_get_version;
> +    tc->request_completed = tpm_crb_isa_request_completed;
>  
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
> -static const TypeInfo tpm_crb_none_info = {
> +static const TypeInfo tpm_crb_isa_info = {
>      .name = TYPE_TPM_CRB,
> -    /* could be TYPE_SYS_BUS_DEVICE (or LPC etc) */
> -    .parent = TYPE_DEVICE,
> +    .parent = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(CRBState),
> -    .class_init  = tpm_crb_none_class_init,
> +    .class_init  = tpm_crb_isa_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_TPM_IF },
>          { }
>      }
>  };
>  
> -static void tpm_crb_none_register(void)
> +static void tpm_crb_isa_register(void)
>  {
> -    type_register_static(&tpm_crb_none_info);
> +    type_register_static(&tpm_crb_isa_info);
>  }
>  
> -type_init(tpm_crb_none_register)
> +type_init(tpm_crb_isa_register)
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index a46663288c..1fd73fe617 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -22,7 +22,7 @@ config TPM_TIS
>  
>  config TPM_CRB
>      bool
> -    depends on TPM && PC
> +    depends on TPM && ISA_BUS
>      select TPM_BACKEND
>  
>  config TPM_SPAPR

On 7/17/23 09:46, Igor Mammedov wrote:
> On Fri, 14 Jul 2023 00:09:21 -0700
> Joelle van Dyne <j@getutm.app> wrote:
> 
>> Since this device is gated to only build for targets with the PC
>> configuration, we should use the ISA bus like with TPM TIS.
> 
> does it affect migration in any way?
>  From guest pov it looks like there a new ISA device will appear
> and then if you do ping pong migration between old - new QEMU will really it work?


> 
> If it will, then commit message here shall describe why it safe and why it works
> 
I would just leave the existing device as-is. This seems safest and we know thta it works.
    Stefan

On Tue, Jul 18, 2023 at 7:16 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/17/23 09:46, Igor Mammedov wrote:
> > On Fri, 14 Jul 2023 00:09:21 -0700
> > Joelle van Dyne <j@getutm.app> wrote:
> >
> >> Since this device is gated to only build for targets with the PC
> >> configuration, we should use the ISA bus like with TPM TIS.
> >
> > does it affect migration in any way?
> >  From guest pov it looks like there a new ISA device will appear
> > and then if you do ping pong migration between old - new QEMU will really it work?
>
>
> >
> > If it will, then commit message here shall describe why it safe and why it works
> >
> I would just leave the existing device as-is. This seems safest and we know thta it works.
>     Stefan

Alexander, do you have any comments here? I know you wanted to move
away from the default bus. The concern is that switching from the
default bus to the ISA bus may cause issues in migration. The current
course of action is to drop this patch.

Hi Joelle,

On 01.08.23 03:46, Joelle van Dyne wrote:
> On Tue, Jul 18, 2023 at 7:16 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>> On 7/17/23 09:46, Igor Mammedov wrote:
>>> On Fri, 14 Jul 2023 00:09:21 -0700
>>> Joelle van Dyne <j@getutm.app> wrote:
>>>
>>>> Since this device is gated to only build for targets with the PC
>>>> configuration, we should use the ISA bus like with TPM TIS.
>>> does it affect migration in any way?
>>>   From guest pov it looks like there a new ISA device will appear
>>> and then if you do ping pong migration between old - new QEMU will really it work?
>>
>>> If it will, then commit message here shall describe why it safe and why it works
>>>
>> I would just leave the existing device as-is. This seems safest and we know thta it works.
>>      Stefan
> Alexander, do you have any comments here? I know you wanted to move
> away from the default bus. The concern is that switching from the
> default bus to the ISA bus may cause issues in migration. The current
> course of action is to drop this patch.


The big problem I have with the CRB device is this code:

https://gitlab.com/qemu-project/qemu/-/blob/master/hw/tpm/tpm_crb.c?ref_type=heads#L297-305

It's a device that maps itself autonomously into system memory. The way 
mapping is supposed to work is that the parent of the device maps it 
into a bus region. If the parent is a machine, it is free to also map it 
into system memory. But a device should not even know what system memory 
is :).

That said, I'm happy if we just introduce a new "sane" sysdev TPM CRB 
device that we use for non-PCs and leave the current layering violating 
one as is.


Alex