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

[Alexander Graf]

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