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
> > > >
> > >
>
