Hey everyone,

I have been working on a project to add support for SPI-based TPMs in QEMU.
Currently, most of our vboot platforms using a SPI-based TPM use the Linux 
SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed 
SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation 
deficiency that prevents bi-directional operations. Thus, in order to connect 
a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
counterpart of the Linux SPI-GPIO driver).

As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,  
I have already tested this implementation locally with our Yosemite-v3.5 platform 
and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80 
for example) to the Yosemite-v3.5 SPI bus containing the TPM.

This patch is an RFC because I have several questions about design. Although the
model is working, I understand there are many areas where the design decision
is not deal (ie. abstracting hard coded GPIO values). Below are some details of the 
patch and specific areas where I would appreciate feedback on how to make this better:
 
hw/arm/aspeed.c: 
I am not sure where the best place is to instantiate the spi_gpio besides the
aspeed_machine_init. Could we add the ability to instantiate it on the command line?

m25p80_transfer8_ex in hw/block/m25p80.c: 
Existing SPI model assumed that the output byte is fully formed, can be passed to 
the SPI device, and the input byte can be returned, all in one operation. With 
SPI-GPIO we don't have the input byte until all 8 bits of the output have been 
shifted out, so we have to prime the MISO with bogus values (0xFF).

MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered 
during testing that when using the mosi pin as the input pin, the mosi value was not 
being updated due to a kernel and aspeed_gpio model optimization. Thus, here we are 
reading the value directly from the gpio controller instead of waiting for the push.

I realize there are Aspeed specifics in the spi_gpio model. To make this extensible, 
is it preferred to make this into a base class and have our Aspeed SPI GPIO extend 
this or we could set up params to pass in the constructor?

Thanks for your review and any direction here would be helpful :) 

Iris Chen (3):
  hw: m25p80: add prereading ability in transfer8
  hw: spi_gpio: add spi gpio model
  hw: aspeed: hook up the spi gpio model

 hw/arm/Kconfig            |   1 +
 hw/arm/aspeed.c           |   5 ++
 hw/block/m25p80.c         |  29 ++++++-
 hw/ssi/Kconfig            |   4 +
 hw/ssi/meson.build        |   1 +
 hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
 hw/ssi/ssi.c              |   4 -
 include/hw/ssi/spi_gpio.h |  38 +++++++++
 include/hw/ssi/ssi.h      |   5 ++
 9 files changed, 248 insertions(+), 5 deletions(-)
 create mode 100644 hw/ssi/spi_gpio.c
 create mode 100644 include/hw/ssi/spi_gpio.h

-- 
2.30.2

Hello Iris,

On 7/29/22 01:23, Iris Chen wrote:
> Hey everyone,
> 
> I have been working on a project to add support for SPI-based TPMs in QEMU.
> Currently, most of our vboot platforms using a SPI-based TPM use the Linux
> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
> deficiency that prevents bi-directional operations. 
aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
HW interface. Your model proposal adds support for a new SPI controller
using bitbang GPIOs. These are really two differents models. I don't see
how you could reuse aspeed_smc for this purpose.

or you mean that Linux is using the SPI-GPIO driver because the Linux
Aspeed SMC driver doesn't match the need ? It is true that the Linux
driver is not generic, it deals with flash devices only. But that's
another problem.

> Thus, in order to connect
> a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
> counterpart of the Linux SPI-GPIO driver).
> 
> As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,
> I have already tested this implementation locally with our Yosemite-v3.5 platform
> and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80
> for example) to the Yosemite-v3.5 SPI bus containing the TPM.
> 
> This patch is an RFC because I have several questions about design. Although the
> model is working, I understand there are many areas where the design decision
> is not deal (ie. abstracting hard coded GPIO values). Below are some details of the
> patch and specific areas where I would appreciate feedback on how to make this better:
>   
> hw/arm/aspeed.c:
> I am not sure where the best place is to instantiate the spi_gpio besides the
> aspeed_machine_init. 

The SPI GPIO device would be a platform device and not a SoC device.
Hence, it should be instantiated at the machine level, like the I2C
device are, using properties to let the model know about the GPIOs
that should be driven to implement the SPI protocol.

Ideally, the state of the GPIO controller pins and the SPI GPIO state
should be shared. I think that's what you are trying to do that with
attribute 'controller_state' in your patch ? But, the way it is done
today, is too tightly coupled (names) with the Aspeed GPIO model to
be generic.

I think we need an intermediate QOM interface, or a base class, to
implement an abstract SPI GPIO model and an Aspeed SPI GPIO model
on top which would be linked to the Aspeed GPIO model of the SoC
in use.

Or we could introduce some kind of generic GPIO controller that
we would link the SPI GPIO model with (using a property). The
Aspeed GPIO model would be of that kind and the SPI GPIO model
would be able to drive the pins using a common interface.
That's another way to do it.

> Could we add the ability to instantiate it on the command line?

It might be complex to identify the QOM object to use as the GPIO
controller from the command line since it is on the SoC and not
a platform device. In that case, an Aspeed SPI GPIO model would
be a better approach. we  would have to peek in the machine/soc to
get a link on the Aspeed GPIO model in the realize routine of the
Aspeed SPI GPIO model.

> m25p80_transfer8_ex in hw/block/m25p80.c:
> Existing SPI model assumed that the output byte is fully formed, can be passed to
> the SPI device, and the input byte can be returned, all in one operation. With
> SPI-GPIO we don't have the input byte until all 8 bits of the output have been
> shifted out, so we have to prime the MISO with bogus values (0xFF).

That's fine I think. We do something similar for dummies.

> MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
> of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
> during testing that when using the mosi pin as the input pin, the mosi value was not
> being updated due to a kernel and aspeed_gpio model optimization. 

ah. We need help from Andrew ! the model should have a mosi pin .

Thanks,

C.

> Thus, here we are
> reading the value directly from the gpio controller instead of waiting for the push.
> 
> I realize there are Aspeed specifics in the spi_gpio model. To make this extensible,
> is it preferred to make this into a base class and have our Aspeed SPI GPIO extend
> this or we could set up params to pass in the constructor?
> 
> Thanks for your review and any direction here would be helpful :)
> 
> Iris Chen (3):
>    hw: m25p80: add prereading ability in transfer8
>    hw: spi_gpio: add spi gpio model
>    hw: aspeed: hook up the spi gpio model
> 
>   hw/arm/Kconfig            |   1 +
>   hw/arm/aspeed.c           |   5 ++
>   hw/block/m25p80.c         |  29 ++++++-
>   hw/ssi/Kconfig            |   4 +
>   hw/ssi/meson.build        |   1 +
>   hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
>   hw/ssi/ssi.c              |   4 -
>   include/hw/ssi/spi_gpio.h |  38 +++++++++
>   include/hw/ssi/ssi.h      |   5 ++
>   9 files changed, 248 insertions(+), 5 deletions(-)
>   create mode 100644 hw/ssi/spi_gpio.c
>   create mode 100644 include/hw/ssi/spi_gpio.h
>

On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
> Hello Iris,
> 
> On 7/29/22 01:23, Iris Chen wrote:
> > Hey everyone,
> > 
> > I have been working on a project to add support for SPI-based TPMs in QEMU.
> > Currently, most of our vboot platforms using a SPI-based TPM use the Linux
> > SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
> > SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
> > deficiency that prevents bi-directional operations.
> aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
> HW interface. Your model proposal adds support for a new SPI controller
> using bitbang GPIOs. These are really two differents models. I don't see
> how you could reuse aspeed_smc for this purpose.
> 
> or you mean that Linux is using the SPI-GPIO driver because the Linux
> Aspeed SMC driver doesn't match the need ? It is true that the Linux
> driver is not generic, it deals with flash devices only. But that's
> another problem.
> 
> > Thus, in order to connect
> > a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
> > counterpart of the Linux SPI-GPIO driver).
> > 
> > As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,
> > I have already tested this implementation locally with our Yosemite-v3.5 platform
> > and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80
> > for example) to the Yosemite-v3.5 SPI bus containing the TPM.
> > 
> > This patch is an RFC because I have several questions about design. Although the
> > model is working, I understand there are many areas where the design decision
> > is not deal (ie. abstracting hard coded GPIO values). Below are some details of the
> > patch and specific areas where I would appreciate feedback on how to make this better:
> > hw/arm/aspeed.c:
> > I am not sure where the best place is to instantiate the spi_gpio besides the
> > aspeed_machine_init.
> 
> The SPI GPIO device would be a platform device and not a SoC device.
> Hence, it should be instantiated at the machine level, like the I2C
> device are, using properties to let the model know about the GPIOs
> that should be driven to implement the SPI protocol.

Agreed, should be like an I2C device.

> 
> Ideally, the state of the GPIO controller pins and the SPI GPIO state
> should be shared. I think that's what you are trying to do that with
> attribute 'controller_state' in your patch ? But, the way it is done
> today, is too tightly coupled (names) with the Aspeed GPIO model to
> be generic.
> 
> I think we need an intermediate QOM interface, or a base class, to
> implement an abstract SPI GPIO model and an Aspeed SPI GPIO model
> on top which would be linked to the Aspeed GPIO model of the SoC
> in use.

Disagree, I feel like we can accomplish this without inheritance.

> 
> Or we could introduce some kind of generic GPIO controller that
> we would link the SPI GPIO model with (using a property). The
> Aspeed GPIO model would be of that kind and the SPI GPIO model
> would be able to drive the pins using a common interface.
> That's another way to do it.

Agree, I would like to go in this direction if at all possible.

> 
> > Could we add the ability to instantiate it on the command line?
> 
> It might be complex to identify the QOM object to use as the GPIO
> controller from the command line since it is on the SoC and not
> a platform device. In that case, an Aspeed SPI GPIO model would
> be a better approach. we  would have to peek in the machine/soc to
> get a link on the Aspeed GPIO model in the realize routine of the
> Aspeed SPI GPIO model.

Hrrrm perhaps, I feel like it shouldn't be that hard though.

- Get a pointer to the QOM object that holds the GPIO's using object
  path or ID. Something similar to '-device ftgmac100,netdev=<id>'
  right?
- Get the GPIO's by name from the QOM object.

In this situation, I think we should be able to get the GPIO controller
object, and then get the IRQ's by the "sysbus-irq[126]"/etc name.

We could refactor the aspeed_gpio.c model to name the IRQ's like the
properties,.  to use "gpioX4" instead of "sysbus-irq[*]".

> 
> > m25p80_transfer8_ex in hw/block/m25p80.c:
> > Existing SPI model assumed that the output byte is fully formed, can be passed to
> > the SPI device, and the input byte can be returned, all in one operation. With
> > SPI-GPIO we don't have the input byte until all 8 bits of the output have been
> > shifted out, so we have to prime the MISO with bogus values (0xFF).
> 
> That's fine I think. We do something similar for dummies.
> 
> > MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
> > of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
> > during testing that when using the mosi pin as the input pin, the mosi value was not
> > being updated due to a kernel and aspeed_gpio model optimization.
> 
> ah. We need help from Andrew ! the model should have a mosi pin .

Not sure if this was clear, but Iris is just saying that she used object
properties to read and write the mosi, miso, and clk pins, rather than
the IRQ's.

Certainly we'd like to use IRQ's instead, but she ran into correctness
problems. Maybe we can investigate that further and fix it.

> 
> Thanks,
> 
> C.
> 
> > Thus, here we are
> > reading the value directly from the gpio controller instead of waiting for the push.
> > 
> > I realize there are Aspeed specifics in the spi_gpio model. To make this extensible,
> > is it preferred to make this into a base class and have our Aspeed SPI GPIO extend
> > this or we could set up params to pass in the constructor?
> > 
> > Thanks for your review and any direction here would be helpful :)
> > 
> > Iris Chen (3):
> >    hw: m25p80: add prereading ability in transfer8
> >    hw: spi_gpio: add spi gpio model
> >    hw: aspeed: hook up the spi gpio model
> > 
> >   hw/arm/Kconfig            |   1 +
> >   hw/arm/aspeed.c           |   5 ++
> >   hw/block/m25p80.c         |  29 ++++++-
> >   hw/ssi/Kconfig            |   4 +
> >   hw/ssi/meson.build        |   1 +
> >   hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
> >   hw/ssi/ssi.c              |   4 -
> >   include/hw/ssi/spi_gpio.h |  38 +++++++++
> >   include/hw/ssi/ssi.h      |   5 ++
> >   9 files changed, 248 insertions(+), 5 deletions(-)
> >   create mode 100644 hw/ssi/spi_gpio.c
> >   create mode 100644 include/hw/ssi/spi_gpio.h
> > 
> 

On 7/29/22 19:30, Peter Delevoryas wrote:
> On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
>> Hello Iris,
>>
>> On 7/29/22 01:23, Iris Chen wrote:
>>> Hey everyone,
>>>
>>> I have been working on a project to add support for SPI-based TPMs in QEMU.
>>> Currently, most of our vboot platforms using a SPI-based TPM use the Linux
>>> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
>>> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
>>> deficiency that prevents bi-directional operations.
>> aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
>> HW interface. Your model proposal adds support for a new SPI controller
>> using bitbang GPIOs. These are really two differents models. I don't see
>> how you could reuse aspeed_smc for this purpose.
>>
>> or you mean that Linux is using the SPI-GPIO driver because the Linux
>> Aspeed SMC driver doesn't match the need ? It is true that the Linux
>> driver is not generic, it deals with flash devices only. But that's
>> another problem.
>>
>>> Thus, in order to connect
>>> a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
>>> counterpart of the Linux SPI-GPIO driver).
>>>
>>> As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,
>>> I have already tested this implementation locally with our Yosemite-v3.5 platform
>>> and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80
>>> for example) to the Yosemite-v3.5 SPI bus containing the TPM.
>>>
>>> This patch is an RFC because I have several questions about design. Although the
>>> model is working, I understand there are many areas where the design decision
>>> is not deal (ie. abstracting hard coded GPIO values). Below are some details of the
>>> patch and specific areas where I would appreciate feedback on how to make this better:
>>> hw/arm/aspeed.c:
>>> I am not sure where the best place is to instantiate the spi_gpio besides the
>>> aspeed_machine_init.
>>
>> The SPI GPIO device would be a platform device and not a SoC device.
>> Hence, it should be instantiated at the machine level, like the I2C
>> device are, using properties to let the model know about the GPIOs
>> that should be driven to implement the SPI protocol.
> 
> Agreed, should be like an I2C device.
> 
>>
>> Ideally, the state of the GPIO controller pins and the SPI GPIO state
>> should be shared. I think that's what you are trying to do that with
>> attribute 'controller_state' in your patch ? But, the way it is done
>> today, is too tightly coupled (names) with the Aspeed GPIO model to
>> be generic.
>>
>> I think we need an intermediate QOM interface, or a base class, to
>> implement an abstract SPI GPIO model and an Aspeed SPI GPIO model
>> on top which would be linked to the Aspeed GPIO model of the SoC
>> in use.
> 
> Disagree, I feel like we can accomplish this without inheritance.
> 
>>
>> Or we could introduce some kind of generic GPIO controller that
>> we would link the SPI GPIO model with (using a property). The
>> Aspeed GPIO model would be of that kind and the SPI GPIO model
>> would be able to drive the pins using a common interface.
>> That's another way to do it.
> 
> Agree, I would like to go in this direction if at all possible.
Let's give it a try then. I would introduce a new QOM interface,
something like  :

     #define TYPE_GPIO_INTERFACE "gpio-interface"
     #define GPIO_INTERFACE(obj)                                     \
         INTERFACE_CHECK(GpioInterface, (obj), TYPE_GPIO_INTERFACE)
     typedef struct GpioInterfaceClass GpioInterfaceClass;
     DECLARE_CLASS_CHECKERS(GpioInterfaceClass, GPIO_INTERFACE,
                            TYPE_GPIO_INTERFACE)
     
     struct GpioInterfaceClass {
         InterfaceClass parent;
         int (*get)(GpioInterface *gi, ...);
         int (*set)(GpioInterface *gi, ...);
         ...
     };

and implement the interface handlers under the AspeedGPIO model.
The SPI GPIO model would have a link to such an interface to drive
the GPIO pins.

See IPMI and XIVE for some more complete models.

>>> Could we add the ability to instantiate it on the command line?
>>
>> It might be complex to identify the QOM object to use as the GPIO
>> controller from the command line since it is on the SoC and not
>> a platform device. In that case, an Aspeed SPI GPIO model would
>> be a better approach. we  would have to peek in the machine/soc to
>> get a link on the Aspeed GPIO model in the realize routine of the
>> Aspeed SPI GPIO model.
> 
> Hrrrm perhaps, I feel like it shouldn't be that hard though.
> 
> - Get a pointer to the QOM object that holds the GPIO's using object
>    path or ID. Something similar to '-device ftgmac100,netdev=<id>'
>    right?

yes. something like that.

> - Get the GPIO's by name from the QOM object.

yes.

> In this situation, I think we should be able to get the GPIO controller
> object, and then get the IRQ's by the "sysbus-irq[126]"/etc name.
> 
> We could refactor the aspeed_gpio.c model to name the IRQ's like the
> properties,.  to use "gpioX4" instead of "sysbus-irq[*]".

we could use qdev_init_gpio_out_named() instead of sysbus_init_irq()
to name them.

>>> m25p80_transfer8_ex in hw/block/m25p80.c:
>>> Existing SPI model assumed that the output byte is fully formed, can be passed to
>>> the SPI device, and the input byte can be returned, all in one operation. With
>>> SPI-GPIO we don't have the input byte until all 8 bits of the output have been
>>> shifted out, so we have to prime the MISO with bogus values (0xFF).
>>
>> That's fine I think. We do something similar for dummies.
>>
>>> MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
>>> of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
>>> during testing that when using the mosi pin as the input pin, the mosi value was not
>>> being updated due to a kernel and aspeed_gpio model optimization.
>>
>> ah. We need help from Andrew ! the model should have a mosi pin .
> 
> Not sure if this was clear, but Iris is just saying that she used object
> properties to read and write the mosi, miso, and clk pins, rather than
> the IRQ's.

The IRQ line is not raised ?

> Certainly we'd like to use IRQ's instead, but she ran into correctness
> problems. Maybe we can investigate that further and fix it.

So much is happening in that mode. We need more trace events in the Aspeed
GPIO model at least an extra in aspeed_gpio_update()

Thanks,

C.

> 
>>
>> Thanks,
>>
>> C.
>>
>>> Thus, here we are
>>> reading the value directly from the gpio controller instead of waiting for the push.
>>>
>>> I realize there are Aspeed specifics in the spi_gpio model. To make this extensible,
>>> is it preferred to make this into a base class and have our Aspeed SPI GPIO extend
>>> this or we could set up params to pass in the constructor?
>>>
>>> Thanks for your review and any direction here would be helpful :)
>>>
>>> Iris Chen (3):
>>>     hw: m25p80: add prereading ability in transfer8
>>>     hw: spi_gpio: add spi gpio model
>>>     hw: aspeed: hook up the spi gpio model
>>>
>>>    hw/arm/Kconfig            |   1 +
>>>    hw/arm/aspeed.c           |   5 ++
>>>    hw/block/m25p80.c         |  29 ++++++-
>>>    hw/ssi/Kconfig            |   4 +
>>>    hw/ssi/meson.build        |   1 +
>>>    hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
>>>    hw/ssi/ssi.c              |   4 -
>>>    include/hw/ssi/spi_gpio.h |  38 +++++++++
>>>    include/hw/ssi/ssi.h      |   5 ++
>>>    9 files changed, 248 insertions(+), 5 deletions(-)
>>>    create mode 100644 hw/ssi/spi_gpio.c
>>>    create mode 100644 include/hw/ssi/spi_gpio.h
>>>
>>

On Sun, 31 Jul 2022, at 06:48, Cédric Le Goater wrote:
> On 7/29/22 19:30, Peter Delevoryas wrote:
>> Certainly we'd like to use IRQ's instead, but she ran into correctness
>> problems. Maybe we can investigate that further and fix it.

Yes, let's not work around problems that we have the ability to fix.

>
> So much is happening in that mode.

Yes, though while there's no-doubt accidental complexity in terms of 
its implementation, the underlying hardware is also quite haphazard and 
we need an approach that scales to the large number of GPIOs it 
provides. So I'd argue there's a bunch of essential complexity involved 
as well.

Do we start with a fresh implementation that allows us to get the 
expected behaviour and then build that out to replace the current 
implementation?

Or, can we add more tests for the existing model to pin down where the 
bugs are?

> We need more trace events in the Aspeed
> GPIO model at least an extra in aspeed_gpio_update()

We can always fall back to this but my hope is we can get better test 
coverage to iron out the bugs. Maybe that gets us a more refined and 
maintainable model implementation along the way.

Cheers,

Andrew

On Mon, Aug 01, 2022 at 11:49:09AM +0930, Andrew Jeffery wrote:
> 
> 
> On Sun, 31 Jul 2022, at 06:48, Cédric Le Goater wrote:
> > On 7/29/22 19:30, Peter Delevoryas wrote:
> >> Certainly we'd like to use IRQ's instead, but she ran into correctness
> >> problems. Maybe we can investigate that further and fix it.
> 
> Yes, let's not work around problems that we have the ability to fix.

+1

> 
> >
> > So much is happening in that mode.
> 
> Yes, though while there's no-doubt accidental complexity in terms of 
> its implementation, the underlying hardware is also quite haphazard and 
> we need an approach that scales to the large number of GPIOs it 
> provides. So I'd argue there's a bunch of essential complexity involved 
> as well.

+1

> 
> Do we start with a fresh implementation that allows us to get the 
> expected behaviour and then build that out to replace the current 
> implementation?
> 
> Or, can we add more tests for the existing model to pin down where the 
> bugs are?

Mmmm good question, I think we might end up doing both. Tests would make
it much easier to develop either way though.

> 
> > We need more trace events in the Aspeed
> > GPIO model at least an extra in aspeed_gpio_update()
> 
> We can always fall back to this but my hope is we can get better test 
> coverage to iron out the bugs. Maybe that gets us a more refined and 
> maintainable model implementation along the way.

+1

> 
> Cheers,
> 
> Andrew
> 

On Sat, Jul 30, 2022 at 11:18:33PM +0200, Cédric Le Goater wrote:
> On 7/29/22 19:30, Peter Delevoryas wrote:
> > On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
> > > Hello Iris,
> > > 
> > > On 7/29/22 01:23, Iris Chen wrote:
> > > > Hey everyone,
> > > > 
> > > > I have been working on a project to add support for SPI-based TPMs in QEMU.
> > > > Currently, most of our vboot platforms using a SPI-based TPM use the Linux
> > > > SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
> > > > SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
> > > > deficiency that prevents bi-directional operations.
> > > aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
> > > HW interface. Your model proposal adds support for a new SPI controller
> > > using bitbang GPIOs. These are really two differents models. I don't see
> > > how you could reuse aspeed_smc for this purpose.
> > > 
> > > or you mean that Linux is using the SPI-GPIO driver because the Linux
> > > Aspeed SMC driver doesn't match the need ? It is true that the Linux
> > > driver is not generic, it deals with flash devices only. But that's
> > > another problem.
> > > 
> > > > Thus, in order to connect
> > > > a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
> > > > counterpart of the Linux SPI-GPIO driver).
> > > > 
> > > > As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,
> > > > I have already tested this implementation locally with our Yosemite-v3.5 platform
> > > > and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80
> > > > for example) to the Yosemite-v3.5 SPI bus containing the TPM.
> > > > 
> > > > This patch is an RFC because I have several questions about design. Although the
> > > > model is working, I understand there are many areas where the design decision
> > > > is not deal (ie. abstracting hard coded GPIO values). Below are some details of the
> > > > patch and specific areas where I would appreciate feedback on how to make this better:
> > > > hw/arm/aspeed.c:
> > > > I am not sure where the best place is to instantiate the spi_gpio besides the
> > > > aspeed_machine_init.
> > > 
> > > The SPI GPIO device would be a platform device and not a SoC device.
> > > Hence, it should be instantiated at the machine level, like the I2C
> > > device are, using properties to let the model know about the GPIOs
> > > that should be driven to implement the SPI protocol.
> > 
> > Agreed, should be like an I2C device.
> > 
> > > 
> > > Ideally, the state of the GPIO controller pins and the SPI GPIO state
> > > should be shared. I think that's what you are trying to do that with
> > > attribute 'controller_state' in your patch ? But, the way it is done
> > > today, is too tightly coupled (names) with the Aspeed GPIO model to
> > > be generic.
> > > 
> > > I think we need an intermediate QOM interface, or a base class, to
> > > implement an abstract SPI GPIO model and an Aspeed SPI GPIO model
> > > on top which would be linked to the Aspeed GPIO model of the SoC
> > > in use.
> > 
> > Disagree, I feel like we can accomplish this without inheritance.
> > 
> > > 
> > > Or we could introduce some kind of generic GPIO controller that
> > > we would link the SPI GPIO model with (using a property). The
> > > Aspeed GPIO model would be of that kind and the SPI GPIO model
> > > would be able to drive the pins using a common interface.
> > > That's another way to do it.
> > 
> > Agree, I would like to go in this direction if at all possible.
> Let's give it a try then. I would introduce a new QOM interface,
> something like  :
> 
>     #define TYPE_GPIO_INTERFACE "gpio-interface"
>     #define GPIO_INTERFACE(obj)                                     \
>         INTERFACE_CHECK(GpioInterface, (obj), TYPE_GPIO_INTERFACE)
>     typedef struct GpioInterfaceClass GpioInterfaceClass;
>     DECLARE_CLASS_CHECKERS(GpioInterfaceClass, GPIO_INTERFACE,
>                            TYPE_GPIO_INTERFACE)
>     struct GpioInterfaceClass {
>         InterfaceClass parent;
>         int (*get)(GpioInterface *gi, ...);
>         int (*set)(GpioInterface *gi, ...);
>         ...
>     };
> 
> and implement the interface handlers under the AspeedGPIO model.
> The SPI GPIO model would have a link to such an interface to drive
> the GPIO pins.
> 
> See IPMI and XIVE for some more complete models.

This sounds good, but I just want to clarify first:

Is it necessary to introduce a GPIO interface?

Or, could we connect the IRQ's just using the existing
QOM/sysbus/object/IRQ infrastructure?

I'll investigate if we can connect the IRQ's without introducing a new
interface. We can continue with this design for now though.

> 
> > > > Could we add the ability to instantiate it on the command line?
> > > 
> > > It might be complex to identify the QOM object to use as the GPIO
> > > controller from the command line since it is on the SoC and not
> > > a platform device. In that case, an Aspeed SPI GPIO model would
> > > be a better approach. we  would have to peek in the machine/soc to
> > > get a link on the Aspeed GPIO model in the realize routine of the
> > > Aspeed SPI GPIO model.
> > 
> > Hrrrm perhaps, I feel like it shouldn't be that hard though.
> > 
> > - Get a pointer to the QOM object that holds the GPIO's using object
> >    path or ID. Something similar to '-device ftgmac100,netdev=<id>'
> >    right?
> 
> yes. something like that.

+1

> 
> > - Get the GPIO's by name from the QOM object.
> 
> yes.

+1

> 
> > In this situation, I think we should be able to get the GPIO controller
> > object, and then get the IRQ's by the "sysbus-irq[126]"/etc name.
> > 
> > We could refactor the aspeed_gpio.c model to name the IRQ's like the
> > properties,.  to use "gpioX4" instead of "sysbus-irq[*]".
> 
> we could use qdev_init_gpio_out_named() instead of sysbus_init_irq()
> to name them.
>

+1, I actually suggested to Iris offline that this change might be
useful regardless.

> 
> > > > m25p80_transfer8_ex in hw/block/m25p80.c:
> > > > Existing SPI model assumed that the output byte is fully formed, can be passed to
> > > > the SPI device, and the input byte can be returned, all in one operation. With
> > > > SPI-GPIO we don't have the input byte until all 8 bits of the output have been
> > > > shifted out, so we have to prime the MISO with bogus values (0xFF).
> > > 
> > > That's fine I think. We do something similar for dummies.
> > > 
> > > > MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
> > > > of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
> > > > during testing that when using the mosi pin as the input pin, the mosi value was not
> > > > being updated due to a kernel and aspeed_gpio model optimization.
> > > 
> > > ah. We need help from Andrew ! the model should have a mosi pin .
> > 
> > Not sure if this was clear, but Iris is just saying that she used object
> > properties to read and write the mosi, miso, and clk pins, rather than
> > the IRQ's.
> 
> The IRQ line is not raised ?

Something like that, yes. She was having trouble following the IRQ level
purely through edge changes. Perhaps this is due to a bug in
aspeed_gpio.c.

> 
> > Certainly we'd like to use IRQ's instead, but she ran into correctness
> > problems. Maybe we can investigate that further and fix it.
> 
> So much is happening in that mode. We need more trace events in the Aspeed
> GPIO model at least an extra in aspeed_gpio_update()

+1

> 
> Thanks,
> 
> C.
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > C.
> > > 
> > > > Thus, here we are
> > > > reading the value directly from the gpio controller instead of waiting for the push.
> > > > 
> > > > I realize there are Aspeed specifics in the spi_gpio model. To make this extensible,
> > > > is it preferred to make this into a base class and have our Aspeed SPI GPIO extend
> > > > this or we could set up params to pass in the constructor?
> > > > 
> > > > Thanks for your review and any direction here would be helpful :)
> > > > 
> > > > Iris Chen (3):
> > > >     hw: m25p80: add prereading ability in transfer8
> > > >     hw: spi_gpio: add spi gpio model
> > > >     hw: aspeed: hook up the spi gpio model
> > > > 
> > > >    hw/arm/Kconfig            |   1 +
> > > >    hw/arm/aspeed.c           |   5 ++
> > > >    hw/block/m25p80.c         |  29 ++++++-
> > > >    hw/ssi/Kconfig            |   4 +
> > > >    hw/ssi/meson.build        |   1 +
> > > >    hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
> > > >    hw/ssi/ssi.c              |   4 -
> > > >    include/hw/ssi/spi_gpio.h |  38 +++++++++
> > > >    include/hw/ssi/ssi.h      |   5 ++
> > > >    9 files changed, 248 insertions(+), 5 deletions(-)
> > > >    create mode 100644 hw/ssi/spi_gpio.c
> > > >    create mode 100644 include/hw/ssi/spi_gpio.h
> > > > 
> > > 
> 

On 7/31/22 00:06, Peter Delevoryas wrote:
> On Sat, Jul 30, 2022 at 11:18:33PM +0200, Cédric Le Goater wrote:
>> On 7/29/22 19:30, Peter Delevoryas wrote:
>>> On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
>>>> Hello Iris,
>>>>
>>>> On 7/29/22 01:23, Iris Chen wrote:
>>>>> Hey everyone,
>>>>>
>>>>> I have been working on a project to add support for SPI-based TPMs in QEMU.
>>>>> Currently, most of our vboot platforms using a SPI-based TPM use the Linux
>>>>> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
>>>>> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
>>>>> deficiency that prevents bi-directional operations.
>>>> aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
>>>> HW interface. Your model proposal adds support for a new SPI controller
>>>> using bitbang GPIOs. These are really two differents models. I don't see
>>>> how you could reuse aspeed_smc for this purpose.
>>>>
>>>> or you mean that Linux is using the SPI-GPIO driver because the Linux
>>>> Aspeed SMC driver doesn't match the need ? It is true that the Linux
>>>> driver is not generic, it deals with flash devices only. But that's
>>>> another problem.
>>>>
>>>>> Thus, in order to connect
>>>>> a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
>>>>> counterpart of the Linux SPI-GPIO driver).
>>>>>
>>>>> As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,
>>>>> I have already tested this implementation locally with our Yosemite-v3.5 platform
>>>>> and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80
>>>>> for example) to the Yosemite-v3.5 SPI bus containing the TPM.
>>>>>
>>>>> This patch is an RFC because I have several questions about design. Although the
>>>>> model is working, I understand there are many areas where the design decision
>>>>> is not deal (ie. abstracting hard coded GPIO values). Below are some details of the
>>>>> patch and specific areas where I would appreciate feedback on how to make this better:
>>>>> hw/arm/aspeed.c:
>>>>> I am not sure where the best place is to instantiate the spi_gpio besides the
>>>>> aspeed_machine_init.
>>>>
>>>> The SPI GPIO device would be a platform device and not a SoC device.
>>>> Hence, it should be instantiated at the machine level, like the I2C
>>>> device are, using properties to let the model know about the GPIOs
>>>> that should be driven to implement the SPI protocol.
>>>
>>> Agreed, should be like an I2C device.
>>>
>>>>
>>>> Ideally, the state of the GPIO controller pins and the SPI GPIO state
>>>> should be shared. I think that's what you are trying to do that with
>>>> attribute 'controller_state' in your patch ? But, the way it is done
>>>> today, is too tightly coupled (names) with the Aspeed GPIO model to
>>>> be generic.
>>>>
>>>> I think we need an intermediate QOM interface, or a base class, to
>>>> implement an abstract SPI GPIO model and an Aspeed SPI GPIO model
>>>> on top which would be linked to the Aspeed GPIO model of the SoC
>>>> in use.
>>>
>>> Disagree, I feel like we can accomplish this without inheritance.
>>>
>>>>
>>>> Or we could introduce some kind of generic GPIO controller that
>>>> we would link the SPI GPIO model with (using a property). The
>>>> Aspeed GPIO model would be of that kind and the SPI GPIO model
>>>> would be able to drive the pins using a common interface.
>>>> That's another way to do it.
>>>
>>> Agree, I would like to go in this direction if at all possible.
>> Let's give it a try then. I would introduce a new QOM interface,
>> something like  :
>>
>>      #define TYPE_GPIO_INTERFACE "gpio-interface"
>>      #define GPIO_INTERFACE(obj)                                     \
>>          INTERFACE_CHECK(GpioInterface, (obj), TYPE_GPIO_INTERFACE)
>>      typedef struct GpioInterfaceClass GpioInterfaceClass;
>>      DECLARE_CLASS_CHECKERS(GpioInterfaceClass, GPIO_INTERFACE,
>>                             TYPE_GPIO_INTERFACE)
>>      struct GpioInterfaceClass {
>>          InterfaceClass parent;
>>          int (*get)(GpioInterface *gi, ...);
>>          int (*set)(GpioInterface *gi, ...);
>>          ...
>>      };
>>
>> and implement the interface handlers under the AspeedGPIO model.
>> The SPI GPIO model would have a link to such an interface to drive
>> the GPIO pins.
>>
>> See IPMI and XIVE for some more complete models.
> 
> This sounds good, but I just want to clarify first:
> 
> Is it necessary to introduce a GPIO interface?

Well, my feeling is that we need an abstract layer to interface the
SPI GPIO model with any model of GPIO controller.
  
> Or, could we connect the IRQ's just using the existing
> QOM/sysbus/object/IRQ infrastructure?

and we would use the QOM canonical path to identify the GPIO pins
and QOM routines to get and set the values ? It looks feasible.
That's how I would do it in a script but not in model.
  
> I'll investigate if we can connect the IRQ's without introducing a new
> interface. We can continue with this design for now though.

OK. Let's see.

>>>>> Could we add the ability to instantiate it on the command line?
>>>>
>>>> It might be complex to identify the QOM object to use as the GPIO
>>>> controller from the command line since it is on the SoC and not
>>>> a platform device. In that case, an Aspeed SPI GPIO model would
>>>> be a better approach. we  would have to peek in the machine/soc to
>>>> get a link on the Aspeed GPIO model in the realize routine of the
>>>> Aspeed SPI GPIO model.
>>>
>>> Hrrrm perhaps, I feel like it shouldn't be that hard though.
>>>
>>> - Get a pointer to the QOM object that holds the GPIO's using object
>>>     path or ID. Something similar to '-device ftgmac100,netdev=<id>'
>>>     right?
>>
>> yes. something like that.
> 
> +1
> 
>>
>>> - Get the GPIO's by name from the QOM object.
>>
>> yes.
> 
> +1
> 
>>
>>> In this situation, I think we should be able to get the GPIO controller
>>> object, and then get the IRQ's by the "sysbus-irq[126]"/etc name.
>>>
>>> We could refactor the aspeed_gpio.c model to name the IRQ's like the
>>> properties,.  to use "gpioX4" instead of "sysbus-irq[*]".
>>
>> we could use qdev_init_gpio_out_named() instead of sysbus_init_irq()
>> to name them.
>>
> 
> +1, I actually suggested to Iris offline that this change might be
> useful regardless.

yes. This change can come as a preliminary.

> 
>>
>>>>> m25p80_transfer8_ex in hw/block/m25p80.c:
>>>>> Existing SPI model assumed that the output byte is fully formed, can be passed to
>>>>> the SPI device, and the input byte can be returned, all in one operation. With
>>>>> SPI-GPIO we don't have the input byte until all 8 bits of the output have been
>>>>> shifted out, so we have to prime the MISO with bogus values (0xFF).
>>>>
>>>> That's fine I think. We do something similar for dummies.
>>>>
>>>>> MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
>>>>> of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
>>>>> during testing that when using the mosi pin as the input pin, the mosi value was not
>>>>> being updated due to a kernel and aspeed_gpio model optimization.
>>>>
>>>> ah. We need help from Andrew ! the model should have a mosi pin .
>>>
>>> Not sure if this was clear, but Iris is just saying that she used object
>>> properties to read and write the mosi, miso, and clk pins, rather than
>>> the IRQ's.
>>
>> The IRQ line is not raised ?
> 
> Something like that, yes. She was having trouble following the IRQ level
> purely through edge changes. Perhaps this is due to a bug in
> aspeed_gpio.c.

That could be it. The HW logic is quite complex. Adding more tests as
suggested by Andrew would help.


Thanks,

C.

  

> 
>>
>>> Certainly we'd like to use IRQ's instead, but she ran into correctness
>>> problems. Maybe we can investigate that further and fix it.
>>
>> So much is happening in that mode. We need more trace events in the Aspeed
>> GPIO model at least an extra in aspeed_gpio_update()
> 
> +1
> 
>>
>> Thanks,
>>
>> C.
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>> Thus, here we are
>>>>> reading the value directly from the gpio controller instead of waiting for the push.
>>>>>
>>>>> I realize there are Aspeed specifics in the spi_gpio model. To make this extensible,
>>>>> is it preferred to make this into a base class and have our Aspeed SPI GPIO extend
>>>>> this or we could set up params to pass in the constructor?
>>>>>
>>>>> Thanks for your review and any direction here would be helpful :)
>>>>>
>>>>> Iris Chen (3):
>>>>>      hw: m25p80: add prereading ability in transfer8
>>>>>      hw: spi_gpio: add spi gpio model
>>>>>      hw: aspeed: hook up the spi gpio model
>>>>>
>>>>>     hw/arm/Kconfig            |   1 +
>>>>>     hw/arm/aspeed.c           |   5 ++
>>>>>     hw/block/m25p80.c         |  29 ++++++-
>>>>>     hw/ssi/Kconfig            |   4 +
>>>>>     hw/ssi/meson.build        |   1 +
>>>>>     hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
>>>>>     hw/ssi/ssi.c              |   4 -
>>>>>     include/hw/ssi/spi_gpio.h |  38 +++++++++
>>>>>     include/hw/ssi/ssi.h      |   5 ++
>>>>>     9 files changed, 248 insertions(+), 5 deletions(-)
>>>>>     create mode 100644 hw/ssi/spi_gpio.c
>>>>>     create mode 100644 include/hw/ssi/spi_gpio.h
>>>>>
>>>>
>>

On Tue, Aug 2, 2022 at 7:25 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 7/31/22 00:06, Peter Delevoryas wrote:
> > On Sat, Jul 30, 2022 at 11:18:33PM +0200, Cédric Le Goater wrote:
> >> On 7/29/22 19:30, Peter Delevoryas wrote:
> >>> On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
> >>>> Hello Iris,
> >>>>
> >>>> On 7/29/22 01:23, Iris Chen wrote:
> >>>>> Hey everyone,
> >>>>>
> >>>>> I have been working on a project to add support for SPI-based TPMs in QEMU.
> >>>>> Currently, most of our vboot platforms using a SPI-based TPM use the Linux
> >>>>> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
> >>>>> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
> >>>>> deficiency that prevents bi-directional operations.
> >>>> aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
> >>>> HW interface. Your model proposal adds support for a new SPI controller
> >>>> using bitbang GPIOs. These are really two differents models. I don't see
> >>>> how you could reuse aspeed_smc for this purpose.
> >>>>
> >>>> or you mean that Linux is using the SPI-GPIO driver because the Linux
> >>>> Aspeed SMC driver doesn't match the need ? It is true that the Linux
> >>>> driver is not generic, it deals with flash devices only. But that's
> >>>> another problem.
> >>>>
> >>>>> Thus, in order to connect
> >>>>> a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
> >>>>> counterpart of the Linux SPI-GPIO driver).
> >>>>>
> >>>>> As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,
> >>>>> I have already tested this implementation locally with our Yosemite-v3.5 platform
> >>>>> and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80
> >>>>> for example) to the Yosemite-v3.5 SPI bus containing the TPM.
> >>>>>
> >>>>> This patch is an RFC because I have several questions about design. Although the
> >>>>> model is working, I understand there are many areas where the design decision
> >>>>> is not deal (ie. abstracting hard coded GPIO values). Below are some details of the
> >>>>> patch and specific areas where I would appreciate feedback on how to make this better:
> >>>>> hw/arm/aspeed.c:
> >>>>> I am not sure where the best place is to instantiate the spi_gpio besides the
> >>>>> aspeed_machine_init.
> >>>>
> >>>> The SPI GPIO device would be a platform device and not a SoC device.
> >>>> Hence, it should be instantiated at the machine level, like the I2C
> >>>> device are, using properties to let the model know about the GPIOs
> >>>> that should be driven to implement the SPI protocol.
> >>>
> >>> Agreed, should be like an I2C device.
> >>>
> >>>>
> >>>> Ideally, the state of the GPIO controller pins and the SPI GPIO state
> >>>> should be shared. I think that's what you are trying to do that with
> >>>> attribute 'controller_state' in your patch ? But, the way it is done
> >>>> today, is too tightly coupled (names) with the Aspeed GPIO model to
> >>>> be generic.
> >>>>
> >>>> I think we need an intermediate QOM interface, or a base class, to
> >>>> implement an abstract SPI GPIO model and an Aspeed SPI GPIO model
> >>>> on top which would be linked to the Aspeed GPIO model of the SoC
> >>>> in use.
> >>>
> >>> Disagree, I feel like we can accomplish this without inheritance.
> >>>
> >>>>
> >>>> Or we could introduce some kind of generic GPIO controller that
> >>>> we would link the SPI GPIO model with (using a property). The
> >>>> Aspeed GPIO model would be of that kind and the SPI GPIO model
> >>>> would be able to drive the pins using a common interface.
> >>>> That's another way to do it.
> >>>
> >>> Agree, I would like to go in this direction if at all possible.
> >> Let's give it a try then. I would introduce a new QOM interface,
> >> something like  :
> >>
> >>      #define TYPE_GPIO_INTERFACE "gpio-interface"
> >>      #define GPIO_INTERFACE(obj)                                     \
> >>          INTERFACE_CHECK(GpioInterface, (obj), TYPE_GPIO_INTERFACE)
> >>      typedef struct GpioInterfaceClass GpioInterfaceClass;
> >>      DECLARE_CLASS_CHECKERS(GpioInterfaceClass, GPIO_INTERFACE,
> >>                             TYPE_GPIO_INTERFACE)
> >>      struct GpioInterfaceClass {
> >>          InterfaceClass parent;
> >>          int (*get)(GpioInterface *gi, ...);
> >>          int (*set)(GpioInterface *gi, ...);
> >>          ...
> >>      };
> >>
> >> and implement the interface handlers under the AspeedGPIO model.
> >> The SPI GPIO model would have a link to such an interface to drive
> >> the GPIO pins.
> >>
> >> See IPMI and XIVE for some more complete models.
> >
> > This sounds good, but I just want to clarify first:
> >
> > Is it necessary to introduce a GPIO interface?
>
> Well, my feeling is that we need an abstract layer to interface the
> SPI GPIO model with any model of GPIO controller.
>
This abstract layer can be in form of virtual func of spi_gpio base class
set_cs(bool), set_clk(), set_mosi(), get_miso().
This give user (normally the board model creator ) the full flexibility as
how to implement these function in inherit class of spi_gpio i.e.
aspeed_spi_gpio

The down side of virtual function "abstract" layer is that user need
to write QEMU code.

Another form of abstract layer is "output pin and properties of GPIO
controller model"
The pro of this is that as long as the GPIO controller provides these
required output pin
and property, the final user can use command line input parameters to
connect the
spi_gpio with the gpio controller.

The required pin and property to controller model will be:
Two named output pins exposed as QEMUIrq, which will be used as SPI_CS
and SPI_CLK.
Two properties which can be used by spi_gpio to get (sample) the level
as SPI_MOSI and
set the level as SPI_MISO.


> > Or, could we connect the IRQ's just using the existing
> > QOM/sysbus/object/IRQ infrastructure?
>
> and we would use the QOM canonical path to identify the GPIO pins
> and QOM routines to get and set the values ? It looks feasible.
> That's how I would do it in a script but not in model.



>
> > I'll investigate if we can connect the IRQ's without introducing a new
> > interface. We can continue with this design for now though.
>
> OK. Let's see.
>
> >>>>> Could we add the ability to instantiate it on the command line?
> >>>>
> >>>> It might be complex to identify the QOM object to use as the GPIO
> >>>> controller from the command line since it is on the SoC and not
> >>>> a platform device. In that case, an Aspeed SPI GPIO model would
> >>>> be a better approach. we  would have to peek in the machine/soc to
> >>>> get a link on the Aspeed GPIO model in the realize routine of the
> >>>> Aspeed SPI GPIO model.
> >>>
> >>> Hrrrm perhaps, I feel like it shouldn't be that hard though.
> >>>
> >>> - Get a pointer to the QOM object that holds the GPIO's using object
> >>>     path or ID. Something similar to '-device ftgmac100,netdev=<id>'
> >>>     right?
> >>
> >> yes. something like that.
> >
> > +1
> >
> >>
> >>> - Get the GPIO's by name from the QOM object.
> >>
> >> yes.
> >
> > +1
> >
> >>
> >>> In this situation, I think we should be able to get the GPIO controller
> >>> object, and then get the IRQ's by the "sysbus-irq[126]"/etc name.
> >>>
> >>> We could refactor the aspeed_gpio.c model to name the IRQ's like the
> >>> properties,.  to use "gpioX4" instead of "sysbus-irq[*]".
> >>
> >> we could use qdev_init_gpio_out_named() instead of sysbus_init_irq()
> >> to name them.
> >>
> >
> > +1, I actually suggested to Iris offline that this change might be
> > useful regardless.
>
> yes. This change can come as a preliminary.
>
> >
> >>
> >>>>> m25p80_transfer8_ex in hw/block/m25p80.c:
> >>>>> Existing SPI model assumed that the output byte is fully formed, can be passed to
> >>>>> the SPI device, and the input byte can be returned, all in one operation. With
> >>>>> SPI-GPIO we don't have the input byte until all 8 bits of the output have been
> >>>>> shifted out, so we have to prime the MISO with bogus values (0xFF).
> >>>>
> >>>> That's fine I think. We do something similar for dummies.
> >>>>
> >>>>> MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
> >>>>> of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
> >>>>> during testing that when using the mosi pin as the input pin, the mosi value was not
> >>>>> being updated due to a kernel and aspeed_gpio model optimization.
> >>>>
> >>>> ah. We need help from Andrew ! the model should have a mosi pin .
> >>>
> >>> Not sure if this was clear, but Iris is just saying that she used object
> >>> properties to read and write the mosi, miso, and clk pins, rather than
> >>> the IRQ's.
> >>
> >> The IRQ line is not raised ?
> >
> > Something like that, yes. She was having trouble following the IRQ level
> > purely through edge changes. Perhaps this is due to a bug in
> > aspeed_gpio.c.
>
> That could be it. The HW logic is quite complex. Adding more tests as
> suggested by Andrew would help.
>
>
> Thanks,
>
> C.
>
>
>
> >
> >>
> >>> Certainly we'd like to use IRQ's instead, but she ran into correctness
> >>> problems. Maybe we can investigate that further and fix it.
> >>
> >> So much is happening in that mode. We need more trace events in the Aspeed
> >> GPIO model at least an extra in aspeed_gpio_update()
> >
> > +1
> >
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>> C.
> >>>>
> >>>>> Thus, here we are
> >>>>> reading the value directly from the gpio controller instead of waiting for the push.
> >>>>>
> >>>>> I realize there are Aspeed specifics in the spi_gpio model. To make this extensible,
> >>>>> is it preferred to make this into a base class and have our Aspeed SPI GPIO extend
> >>>>> this or we could set up params to pass in the constructor?
> >>>>>
> >>>>> Thanks for your review and any direction here would be helpful :)
> >>>>>
> >>>>> Iris Chen (3):
> >>>>>      hw: m25p80: add prereading ability in transfer8
> >>>>>      hw: spi_gpio: add spi gpio model
> >>>>>      hw: aspeed: hook up the spi gpio model
> >>>>>
> >>>>>     hw/arm/Kconfig            |   1 +
> >>>>>     hw/arm/aspeed.c           |   5 ++
> >>>>>     hw/block/m25p80.c         |  29 ++++++-
> >>>>>     hw/ssi/Kconfig            |   4 +
> >>>>>     hw/ssi/meson.build        |   1 +
> >>>>>     hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
> >>>>>     hw/ssi/ssi.c              |   4 -
> >>>>>     include/hw/ssi/spi_gpio.h |  38 +++++++++
> >>>>>     include/hw/ssi/ssi.h      |   5 ++
> >>>>>     9 files changed, 248 insertions(+), 5 deletions(-)
> >>>>>     create mode 100644 hw/ssi/spi_gpio.c
> >>>>>     create mode 100644 include/hw/ssi/spi_gpio.h
> >>>>>
> >>>>
> >>
>

Thanks everyone for the insightful feedback! This is really helpful for me. 

I am taking a look at all the comments now and will investigate into it. 

Best, 
Iris 

On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
> Hello Iris,
> 
> On 7/29/22 01:23, Iris Chen wrote:

> > MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
> > of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered
> > during testing that when using the mosi pin as the input pin, the mosi value was not
> > being updated due to a kernel and aspeed_gpio model optimization. 
> 
> ah. We need help from Andrew ! the model should have a mosi pin .

This discussion about SMC reminded me of something that might be leading
to the issues we're seeing.  Our hardware implementation uses the same
GPIOs as one of the SMCs and doesn't use the SMC.  It could be that both
QEMU models (the SPI-GPIO and the SMC) are trying to grab the same
GPIOs.

-- 
Patrick Williams

On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
> Hello Iris,
> 
> On 7/29/22 01:23, Iris Chen wrote:
> > Currently, most of our vboot platforms using a SPI-based TPM use the Linux
> > SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
> > SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
> > deficiency that prevents bi-directional operations. 
> aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
> HW interface. Your model proposal adds support for a new SPI controller
> using bitbang GPIOs. These are really two differents models. I don't see
> how you could reuse aspeed_smc for this purpose.

(I don't think there was anything here that proposed reusing the QEMU
 aspeed_smc model, but it might have been poorly worded).

> or you mean that Linux is using the SPI-GPIO driver because the Linux
> Aspeed SMC driver doesn't match the need ? It is true that the Linux
> driver is not generic, it deals with flash devices only. But that's
> another problem.

We actually mean that the _hardware_ has a deficiency, not any of the
code for it.  The Aspeed SPI controller has a single byte FIFO in its
implementation, which can only read or write in a single operation.  It
is *impossible* to perform bi-directional access with it (ie. you cannot
write the MOSI and read the MISO in the same transaction).  This is
broken for many SPI protocols, other than flash devices, including the one
used for TPMs.

In order to connect to SPI-based TPMs on an Aspeed chip, you have to use
the SPI-GPIO method.

-- 
Patrick Williams

On 7/29/22 16:38, Patrick Williams wrote:
> On Fri, Jul 29, 2022 at 03:25:55PM +0200, Cédric Le Goater wrote:
>> Hello Iris,
>>
>> On 7/29/22 01:23, Iris Chen wrote:
>>> Currently, most of our vboot platforms using a SPI-based TPM use the Linux
>>> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed
>>> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation
>>> deficiency that prevents bi-directional operations.
>> aspeed_smc models the Aspeed FMC/SPI controllers which have a well defined
>> HW interface. Your model proposal adds support for a new SPI controller
>> using bitbang GPIOs. These are really two differents models. I don't see
>> how you could reuse aspeed_smc for this purpose.
> 
> (I don't think there was anything here that proposed reusing the QEMU
>   aspeed_smc model, but it might have been poorly worded).

yeah. That's fine. I was trying to see if we could ease Iris work with
a fix in some driver/model but I didn't understand where.

>> or you mean that Linux is using the SPI-GPIO driver because the Linux
>> Aspeed SMC driver doesn't match the need ? It is true that the Linux
>> driver is not generic, it deals with flash devices only. But that's
>> another problem.
> 
> We actually mean that the _hardware_ has a deficiency, not any of the
> code for it.  The Aspeed SPI controller has a single byte FIFO in its
> implementation, which can only read or write in a single operation.  It
> is *impossible* to perform bi-directional access with it (ie. you cannot
> write the MOSI and read the MISO in the same transaction).  This is
> broken for many SPI protocols, other than flash devices, including the one
> used for TPMs.
> 
> In order to connect to SPI-based TPMs on an Aspeed chip, you have to use
> the SPI-GPIO method.

Ok. Thanks for the clarification.

C.

On Thu, Jul 28, 2022 at 04:23:19PM -0700, Iris Chen wrote:
> Hey everyone,
> 
> I have been working on a project to add support for SPI-based TPMs in QEMU.
> Currently, most of our vboot platforms using a SPI-based TPM use the Linux 
> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed 
> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an implementation 
> deficiency that prevents bi-directional operations. Thus, in order to connect 
> a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
> counterpart of the Linux SPI-GPIO driver).
> 
> As we use SPI-based TPMs on many of our BMCs for the secure-boot implementation,  
> I have already tested this implementation locally with our Yosemite-v3.5 platform 
> and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR (m25p80 
> for example) to the Yosemite-v3.5 SPI bus containing the TPM.
> 
> This patch is an RFC because I have several questions about design. Although the
> model is working, I understand there are many areas where the design decision
> is not deal (ie. abstracting hard coded GPIO values). Below are some details of the 
> patch and specific areas where I would appreciate feedback on how to make this better:
>  
> hw/arm/aspeed.c: 
> I am not sure where the best place is to instantiate the spi_gpio besides the
> aspeed_machine_init. Could we add the ability to instantiate it on the command line?

Yes, hypothetically I think it would be something like this:

-device spi-gpio,miso=aspeed.gpio.gpioX4,mosi=aspeed.gpio.gpioX5,id=foo
-device n25q00,bus=foo,drive=bar
-drive file=bar.mtd,format=raw,if=mtd,id=bar

Something like that? I haven't really looked into the details. I think
it requires work in several other places though.

> 
> m25p80_transfer8_ex in hw/block/m25p80.c: 
> Existing SPI model assumed that the output byte is fully formed, can be passed to 
> the SPI device, and the input byte can be returned, all in one operation. With 
> SPI-GPIO we don't have the input byte until all 8 bits of the output have been 
> shifted out, so we have to prime the MISO with bogus values (0xFF).

Perhaps the Aspeed FMC model needs to support asynchronous transfers?
(Is the M25P80 model not asynchronous already?) I'm still skeptical that
the m25p80_transfer8_ex solution is appropriate.

> 
> MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime value
> of the gpio for input bits to prevent bugs with caching the mosi value. It was discovered 
> during testing that when using the mosi pin as the input pin, the mosi value was not 
> being updated due to a kernel and aspeed_gpio model optimization. Thus, here we are 
> reading the value directly from the gpio controller instead of waiting for the push.
> 
> I realize there are Aspeed specifics in the spi_gpio model. To make this extensible, 
> is it preferred to make this into a base class and have our Aspeed SPI GPIO extend 
> this or we could set up params to pass in the constructor?

Actually, I would hope that there won't be any inheritance here. The
kernel driver doesn't have some kind of inheritance implementation, for
example.

> 
> Thanks for your review and any direction here would be helpful :) 
> 
> Iris Chen (3):
>   hw: m25p80: add prereading ability in transfer8
>   hw: spi_gpio: add spi gpio model
>   hw: aspeed: hook up the spi gpio model
> 
>  hw/arm/Kconfig            |   1 +
>  hw/arm/aspeed.c           |   5 ++
>  hw/block/m25p80.c         |  29 ++++++-
>  hw/ssi/Kconfig            |   4 +
>  hw/ssi/meson.build        |   1 +
>  hw/ssi/spi_gpio.c         | 166 ++++++++++++++++++++++++++++++++++++++
>  hw/ssi/ssi.c              |   4 -
>  include/hw/ssi/spi_gpio.h |  38 +++++++++
>  include/hw/ssi/ssi.h      |   5 ++
>  9 files changed, 248 insertions(+), 5 deletions(-)
>  create mode 100644 hw/ssi/spi_gpio.c
>  create mode 100644 include/hw/ssi/spi_gpio.h
> 
> -- 
> 2.30.2
>