Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions
On 8/6/22 17:32, Vladimir Sementsov-Ogievskiy wrote: if I understand correctly you suggest: .prepare = check and then change aiocontext .abort = revert aiocontext change .commit = nothing Yes. And that's is how it used actually now in transactions, for example: bdrv_attach_child_common (which is .prepare) calls bdrv_try_set_aio_context() (which is check and then change) bdrv_attach_child_common_abort (which is .abort) calls bdrv_try_set_aio_context() to revert .prepare I see your point, but I think this can be solved just with a doc comment explaining why the transaction is "local". This is already clear in bdrv_child_try_change_aio_context (which doesn't take a Transaction*), it can be documented in bdrv_child_change_aio_context as well. After all, the point of this series is just to avoid code duplication in the two visits of the graph, and secondarily to unify the .can_set and .set callbacks into one. We could do it just as well without transaction with just a GList of BdrvChild* (keeping the callbcks separate); what the Transaction API gives is a little more abstraction, by not having to do linked list manipulation by hand. To be honest I also don't like very much the placement of the drained_begin/drained_end in bdrv_change_aio_context. But if the needs arises to call bdrv_change_aio_context within another transaction, I think they can be pulled relatively easily (as a subtree drain) in bdrv_child_try_change_aio_context or in its callers. For now, this series is already an improvement on several counts. Paolo
Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation
On 8/3/22 04:52, Cédric Le Goater wrote:
On 8/3/22 04:32, Iris Chen wrote:
From: Iris Chen
A commit log telling us about this new device would be good to have.
Signed-off-by: Iris Chen
---
configs/devices/arm-softmmu/default.mak | 1 +
hw/arm/Kconfig | 5 +
hw/tpm/Kconfig | 5 +
hw/tpm/meson.build | 1 +
hw/tpm/tpm_tis_spi.c | 311
include/sysemu/tpm.h | 3 +
6 files changed, 326 insertions(+)
create mode 100644 hw/tpm/tpm_tis_spi.c
diff --git a/configs/devices/arm-softmmu/default.mak
b/configs/devices/arm-softmmu/default.mak
index 6985a25377..80d2841568 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
CONFIG_SEMIHOSTING=y
CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
CONFIG_ALLWINNER_H3=y
+CONFIG_FBOBMC_AST=y
I don't think this extra config is useful for now
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 15fa79afd3..193decaec1 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -458,6 +458,11 @@ config ASPEED_SOC
select PMBUS
select MAX31785
+config FBOBMC_AST
+ bool
+ select ASPEED_SOC
+ select TPM_TIS_SPI
+
config MPS2
bool
imply I2C_DEVICES
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 29e82f3c92..370a43f045 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
depends on TPM
select TPM_TIS
+config TPM_TIS_SPI
+ bool
+ depends on TPM
+ select TPM_TIS
+
config TPM_TIS
bool
depends on TPM
diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index 1c68d81d6a..1a057f4e36 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true:
files('tpm_tis_common.c'))
softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true:
files('tpm_tis_isa.c'))
softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true:
files('tpm_tis_sysbus.c'))
softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
+softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true:
files('tpm_tis_spi.c'))
specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true:
files('tpm_ppi.c'))
specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true:
files('tpm_ppi.c'))
diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
new file mode 100644
index 00..c98ddcfddb
--- /dev/null
+++ b/hw/tpm/tpm_tis_spi.c
@@ -0,0 +1,311 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "hw/acpi/tpm.h"
+#include "tpm_prop.h"
+#include "tpm_tis.h"
+#include "qom/object.h"
+#include "hw/ssi/ssi.h"
+#include "hw/ssi/spi_gpio.h"
+
+#define TPM_TIS_SPI_ADDR_BYTES 3
+#define SPI_WRITE 0
+
+typedef enum {
+ TIS_SPI_PKT_STATE_DEACTIVATED = 0,
+ TIS_SPI_PKT_STATE_START,
+ TIS_SPI_PKT_STATE_ADDRESS,
+ TIS_SPI_PKT_STATE_DATA_WR,
+ TIS_SPI_PKT_STATE_DATA_RD,
+ TIS_SPI_PKT_STATE_DONE,
+} TpmTisSpiPktState;
+
+union TpmTisRWSizeByte {
+ uint8_t byte;
+ struct {
+ uint8_t data_expected_size:6;
+ uint8_t resv:1;
+ uint8_t rwflag:1;
+ };
+};
+
+union TpmTisSpiHwAddr {
+ hwaddr addr;
+ uint8_t bytes[sizeof(hwaddr)];
+};
+
+union TpmTisSpiData {
+ uint32_t data;
+ uint8_t bytes[64];
+};
+
+struct TpmTisSpiState {
+ /*< private >*/
+ SSIPeripheral parent_obj;
+
+ /*< public >*/
+ TPMState tpm_state; /* not a QOM object */
+ TpmTisSpiPktState tpm_tis_spi_state;
+
+ union TpmTisRWSizeByte first_byte;
+ union TpmTisSpiHwAddr addr;
+ union TpmTisSpiData data;
Are these device registers ? I am not sure the unions are very useful.
+ uint32_t data_size;
+ uint8_t data_idx;
+ uint8_t addr_idx; >> +};
I suppose that these registers will also have to be stored as part of
the device state (for suspend/resume).
+/*
+ * Pre-reading logic for transfer:
+ * This is to fix the transaction between reading and writing.
+ * The first byte is arbitrarily inserted so we need to
+ * shift the all the output bytes (timeline) one byte right.
-> shift all the output bytes (timeline) one byte to the right
+
+static void tpm_tis_spi_realizefn(SSIPeripheral *ss, Error **errp)
+{
+ TpmTisSpiState *sbdev = TPM_TIS_SPI(ss);
+
+ if (!tpm_find()) {
+ error_setg(errp, "at most one TPM device is permitted");
+ return;
+ }
+
+ if (!sbdev->tpm_state.be_driver) {
+ error_setg(errp, "'tpmdev' property is required");
+ return;
+ }
+
+ DeviceState *spi_gpio = qdev_find_recursive(sysbus_get_default(),
+ TYPE_SPI_GPIO);
+ qdev_connect_gpio_out_named(spi_gpio,
+ "SPI_CS_out", 0,
+ qdev_get_gpio_in_named(DEVICE(ss),
+ SSI_GPIO_CS, 0));
Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation
On 8/3/22 04:52, Cédric Le Goater wrote:
On 8/3/22 04:32, Iris Chen wrote:
From: Iris Chen
+++ b/hw/tpm/tpm_tis_spi.c
@@ -0,0 +1,311 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "hw/acpi/tpm.h"
+#include "tpm_prop.h"
+#include "tpm_tis.h"
+#include "qom/object.h"
+#include "hw/ssi/ssi.h"
+#include "hw/ssi/spi_gpio.h"
+
+#define TPM_TIS_SPI_ADDR_BYTES 3
+#define SPI_WRITE 0
+
+typedef enum {
+ TIS_SPI_PKT_STATE_DEACTIVATED = 0,
+ TIS_SPI_PKT_STATE_START,
+ TIS_SPI_PKT_STATE_ADDRESS,
+ TIS_SPI_PKT_STATE_DATA_WR,
+ TIS_SPI_PKT_STATE_DATA_RD,
+ TIS_SPI_PKT_STATE_DONE,
+} TpmTisSpiPktState;
+
+union TpmTisRWSizeByte {
+ uint8_t byte;
+ struct {
+ uint8_t data_expected_size:6;
+ uint8_t resv:1;
+ uint8_t rwflag:1;
+ };
I think it would be better to define a mask for the number of bytes and
a flag for read/write rather than using bitfields. It should better for
portability.
Stefan
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Marc-André Lureau writes: > Hi > > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster wrote: > >> marcandre.lur...@redhat.com writes: >> >> > From: Marc-André Lureau >> > >> > This is just moving qapi-gen.py and related subdir to qemu-common, to >> > ease review and proceed step by step. The following patches will move >> > related necessary code, tests etc. >> > >> > Signed-off-by: Marc-André Lureau >> >> As moved files tend to become low-level annoyances for a long time, I'd >> like to understand why you want to move them. The commit message says >> "to ease review", which I suspect isn't the real reason. Perhaps you >> explained all that elsewhere already, but I missed it. >> >> >> > The end goal is to split some projects, such as qemu-ga, to standalone > meson projects/subprojects. We will be able to build them independently > from the rest of QEMU, and later on perhaps handle them outside of QEMU > main repository. To achieve this, I first introduce a qemu-common > subproject, where qapi and common units are provided. You can check > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek at > current end result. I worry this move of the QAPI generator code into subjprojects/common/scripts/qapi/ will be followed by a move into its own subproject. Ignorant question: could we turn the QAPI generator into a subproject in place? > I said "to ease review and proceed step by step" simply because there are > no other changes: I don't move the rest of the qapi code & tests all > together, it's in the subsequent series. I'd recommend to provide a bit more context in the commit message, even if you copy it to several messages in a row. Our future selves will likely be grateful.
