qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC 0/3] Add Generic SPI GPIO model


From: Cédric Le Goater
Subject: Re: [RFC 0/3] Add Generic SPI GPIO model
Date: Fri, 29 Jul 2022 15:25:55 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]