Re: AST2600 support in QEMU
On Tue, Aug 9, 2022 at 10:51 PM Cédric Le Goater wrote: > > Hello, > > On 8/10/22 04:37, Joel Stanley wrote: > > Hello Shivi, > > > > I've added others to cc who may have some input. > > > > On Tue, 9 Aug 2022 at 21:38, Shivi Fotedar wrote: > >> > >> Hello, we are looking for support for few features for AST2600 in QEMU, > >> specifically > >> > >> PCIe RC support so BMC can talk to downstream devices for management > >> functions. Normally the RC is the host CPU, BMC and the devices to be managed, which support MCTP-over-PCIe will be the endpoint (downstream) device as BMC. The MCTP message Peer transaction between BMC and managed device will using route-by-Id to RC(host) then down to endpoint. I am referring to DMTF DSP0238 spec. section 6.4 So what does the "PCIe RC support" means? the BMC will be the PCIe RC? or BMC will be PCIe-Endpoint connect to host PCIe RC. > > > > I haven't seen any PCIe work done yet. > > I haven't either. There is clearly a need now that we are moving > away from LPC. > > >> MCTP controller to run MCTP protocol on top of PCIe or I2C. > > > > What work would be required to do this on top of i2c? > > I think Jonathan and Klaus worked on this. See : > >https://lore.kernel.org/qemu-devel/20220525121422.3...@huawei.com/ > > >> I2C slave so BMC can talk to host CPU QEMU for IPMI > > > > Some support for slave mode was merged in v7.1. > > yes. > > Peter D. experimented with IPMI. See : > >https://lore.kernel.org/qemu-devel/20220630045133.32251-14...@pjd.dev/ > > We also merged a new machine including a BMC ast2600 running OpenBMC > and an ast1030 SoC running OpenBIC. Work to interconnect them on the > same I2C bus is in progress. > > Thanks, > > C. >
Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation
On Thu, Aug 4, 2022 at 4:21 PM Peter Delevoryas wrote:
>
> On Thu, Aug 04, 2022 at 11:07:10AM -0700, Dan Zhang wrote:
> > On Wed, Aug 3, 2022 at 10:30 AM Peter Delevoryas wrote:
> > >
> > > On Wed, Aug 03, 2022 at 10:52:23AM +0200, 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/qd
Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation
On Wed, Aug 3, 2022 at 10:30 AM Peter Delevoryas wrote:
>
> On Wed, Aug 03, 2022 at 10:52:23AM +0200, 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.
>
> +1, I don't think we should be using unions, instead we should split out
> all the relevant fields we want to store and use extract32/deposit32/etc
> if necessary.
These union is used to saving us code to extract the bits from first byte
and assembling the hwaddr and unint32_t data from bytes.
I think as the bitfields has
Re: [RFC 0/3] Add Generic SPI GPIO model
On Tue, Aug 2, 2022 at 7:25 AM Cédric Le Goater 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
Re: [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
On Fri, Jun 17, 2022 at 03:02:45PM -0700, Iris Chen wrote:
> Signed-off-by: Iris Chen
> ---
> Thanks everyone for your comments. This is a v3 patch that addresses all
> suggestions (moving write_enable to decode_new_cmd).
> I am waiting on some feedback from Dan's (dz4l...@gmail.com) patch
> regarding adding a STATE_STANDBY state.
The STATE_STANDBY can be use to handle the current code's potential issue:
The two data bytes of the WRSR instruction will get interprted as the
new command when abort because of HPM is on. If the data bytes coincident
be a legimtate command, the model will introduce tricky behavior when
HPM is on.
BRs
Dan
>
> Currently, all tests are passing.
>
> hw/block/m25p80.c | 77 ++-
> 1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..12a59ca57c 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -27,12 +27,14 @@
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> #include "hw/ssi/ssi.h"
> +#include "hw/irq.h"
> #include "migration/vmstate.h"
> #include "qemu/bitops.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> +#include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
>
> @@ -472,11 +474,13 @@ struct Flash {
> uint8_t spansion_cr2v;
> uint8_t spansion_cr3v;
> uint8_t spansion_cr4v;
> +bool wp_level;
> bool write_enable;
> bool four_bytes_address_mode;
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> +bool status_register_write_disabled;
> uint8_t ear;
>
> int64_t dirty_page;
> @@ -723,6 +727,8 @@ static void complete_collecting_data(Flash *s)
> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> break;
> case WRSR:
> +s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> +
> switch (get_man(s)) {
> case MAN_SPANSION:
> s->quad_enable = !!(s->data[1] & 0x02);
> @@ -1165,22 +1171,34 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> break;
>
> case WRSR:
> -if (s->write_enable) {
> -switch (get_man(s)) {
> -case MAN_SPANSION:
> -s->needed_bytes = 2;
> -s->state = STATE_COLLECTING_DATA;
> -break;
> -case MAN_MACRONIX:
> -s->needed_bytes = 2;
> -s->state = STATE_COLLECTING_VAR_LEN_DATA;
> -break;
> -default:
> -s->needed_bytes = 1;
> -s->state = STATE_COLLECTING_DATA;
> -}
> -s->pos = 0;
> +/*
> + * If WP# is low and status_register_write_disabled is high,
> + * status register writes are disabled.
> + * This is also called "hardware protected mode" (HPM). All other
> + * combinations of the two states are called "software protected
> mode"
> + * (SPM), and status register writes are permitted.
> + */
> +if ((s->wp_level == 0 && s->status_register_write_disabled)
> +|| !s->write_enable) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: Status register write is disabled!\n");
> +break;
> }
> +
> +switch (get_man(s)) {
> +case MAN_SPANSION:
> +s->needed_bytes = 2;
> +s->state = STATE_COLLECTING_DATA;
> +break;
> +case MAN_MACRONIX:
> +s->needed_bytes = 2;
> +s->state = STATE_COLLECTING_VAR_LEN_DATA;
> +break;
> +default:
> +s->needed_bytes = 1;
> +s->state = STATE_COLLECTING_DATA;
> +}
> +s->pos = 0;
> break;
>
> case WRDI:
> @@ -1195,6 +1213,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>
> case RDSR:
> s->data[0] = (!!s->write_enable) << 1;
> +s->data[0] |= (!!s->status_register_write_disabled) << 7;
> +
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> }
> @@ -1484,6 +1504,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss,
> uint32_t tx)
> return r;
> }
>
> +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int
> level)
> +{
> +Flash *s = M25P80(opaque);
> +/* WP# is just a single pin. */
> +assert(n == 0);
> +s->wp_level = !!level;
> +}
> +
> static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> {
> Flash *s = M25P80(ss);
> @@ -1515,12 +1543,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error
> **errp)
> s->storage = blk_blockalign(NULL, s->size);
> memset(s->storage, 0xFF, s->size);
> }
> +
> +qdev_init_gpio_in_named(DEVICE(s),
> +
[PATCH] hw:m25p80: Add STATE_STANDBY command state
HW normally will switch it to stand by mode when receive incorrect
command.
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
```
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
```
Add STATE_STANDBY CMDState and let the device ignore all input and keep
SO as HIZ (output 1)
Signed-off-by: Dan Zhang
---
A usage of this new state can be aborting in HPM checking
or unknown command code received.
hw/block/m25p80.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index b6bd430a99..9f89773b11 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -423,6 +423,7 @@ typedef enum {
STATE_COLLECTING_DATA,
STATE_COLLECTING_VAR_LEN_DATA,
STATE_READING_DATA,
+STATE_STANDBY,
} CMDState;
typedef enum {
@@ -1472,6 +1473,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss,
uint32_t tx)
s->cur_addr, (uint8_t)tx);
switch (s->state) {
+case STATE_STANDBY:
+r = 0x; /* StandBy state SO shall be HiZ */
+break;
case STATE_PAGE_PROGRAM:
trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
--
2.34.3
Re: [PATCH] hw:w25p80: Add STATE_STANDBY to handle incorrect command
Hi Cedric,
I am sorry that accidently submit a pre-view code change as a patch using the
git-sendmail.
I originally mean to copy the following code in email reply and let
commnity get better understand my proposal.
Let me submit a formal patch in seperate thread. And will remove the
code using this STATE_STANDBY state, as those code shall be in @iris WP#
patch.
BRs
Dan
On Tue, Jun 14, 2022 at 09:02:46AM -0700, Dan Zhang wrote:
> ---
> hw/block/m25p80.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index b6bd430a99..3bb0466dca 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -423,6 +423,7 @@ typedef enum {
> STATE_COLLECTING_DATA,
> STATE_COLLECTING_VAR_LEN_DATA,
> STATE_READING_DATA,
> +STATE_STANDBY,
> } CMDState;
>
> typedef enum {
> @@ -1218,6 +1219,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> || !s->write_enable) {
> qemu_log_mask(LOG_GUEST_ERROR,
>"M25P80: Status register write is disabled!\n");
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: switch to standby, re-aseert CS to
> reactivate \n");
> + s->state = STATE_STANDBY;
> break;
> }
>
> @@ -1472,6 +1476,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss,
> uint32_t tx)
>s->cur_addr, (uint8_t)tx);
>
> switch (s->state) {
> +case STATE_STANDBY:
> + r = 0x; /* StandBy state SO shall be HiZ */
> + break;
>
> case STATE_PAGE_PROGRAM:
> trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
> --
> 2.34.3
>
[PATCH] hw:w25p80: Add STATE_STANDBY to handle incorrect command
---
hw/block/m25p80.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index b6bd430a99..3bb0466dca 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -423,6 +423,7 @@ typedef enum {
STATE_COLLECTING_DATA,
STATE_COLLECTING_VAR_LEN_DATA,
STATE_READING_DATA,
+STATE_STANDBY,
} CMDState;
typedef enum {
@@ -1218,6 +1219,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
|| !s->write_enable) {
qemu_log_mask(LOG_GUEST_ERROR,
"M25P80: Status register write is disabled!\n");
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: switch to standby, re-aseert CS to
reactivate \n");
+ s->state = STATE_STANDBY;
break;
}
@@ -1472,6 +1476,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss,
uint32_t tx)
s->cur_addr, (uint8_t)tx);
switch (s->state) {
+case STATE_STANDBY:
+ r = 0x; /* StandBy state SO shall be HiZ */
+ break;
case STATE_PAGE_PROGRAM:
trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
--
2.34.3
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
On Mon, Jun 13, 2022 at 10:45:34PM -0700, Dan Zhang wrote:
> Just find out how to use mutt to reply all in the thread.
> repeat the previous comments. Add STATE_HIZ to handle decode_new_command
> aborting gracefully.
>
> On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
> >
> >
> > > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> > > wrote:
> > >
> > > Hi Iris,
> > >
> > > Looks good some, a couple of comments below.
> > >
> > > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> > >> From: Iris Chen
> > >>
> > >> Signed-off-by: Iris Chen
> > >> ---
> > >> Addressed all comments from V1. The biggest change: removed
> > >> object_class_property_add.
> > >>
> > >> hw/block/m25p80.c | 37 +++
> > >> tests/qtest/aspeed_smc-test.c | 2 ++
> > >> 2 files changed, 39 insertions(+)
> > >>
> > >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > >> index 81ba3da4df..1a20bd55d4 100644
> > >> --- a/hw/block/m25p80.c
> > >> +++ b/hw/block/m25p80.c
> > >> @@ -27,12 +27,14 @@
> > >> #include "hw/qdev-properties.h"
> > >> #include "hw/qdev-properties-system.h"
> > >> #include "hw/ssi/ssi.h"
> > >> +#include "hw/irq.h"
> > >> #include "migration/vmstate.h"
> > >> #include "qemu/bitops.h"
> > >> #include "qemu/log.h"
> > >> #include "qemu/module.h"
> > >> #include "qemu/error-report.h"
> > >> #include "qapi/error.h"
> > >> +#include "qapi/visitor.h"
> > >> #include "trace.h"
> > >> #include "qom/object.h"
> > >>
> > >> @@ -472,11 +474,13 @@ struct Flash {
> > >> uint8_t spansion_cr2v;
> > >> uint8_t spansion_cr3v;
> > >> uint8_t spansion_cr4v;
> > >> +bool wp_level;
> > >> bool write_enable;
> > >> bool four_bytes_address_mode;
> > >> bool reset_enable;
> > >> bool quad_enable;
> > >> bool aai_enable;
> > >> +bool status_register_write_disabled;
> > >> uint8_t ear;
> > >>
> > >> int64_t dirty_page;
> > >> @@ -723,6 +727,21 @@ static void complete_collecting_data(Flash *s)
> > >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> > >> break;
> > >> case WRSR:
> > >> +/*
> > >> + * If WP# is low and status_register_write_disabled is high,
> > >> + * status register writes are disabled.
> > >> + * This is also called "hardware protected mode" (HPM). All
> > >> other
> > >> + * combinations of the two states are called "software
> > >> protected mode"
> > >> + * (SPM), and status register writes are permitted.
> > >> + */
> > >> +if ((s->wp_level == 0 && s->status_register_write_disabled)
> > >> +|| !s->write_enable) {
> > >
> > > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > > command, otherwise the state machinery will not advance to this function
> > > (meaning that above check for !s->write_enable will never hit as far as I
> > > can
> > > tell). A suggestion is to move the check for wp_level and
> > > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
> >
> > Oh good catch! Yes actually, in our fork, we also removed the write_enable
> > guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> > or both checks in complete_collecting_data.
> >
> > I think we had some difficulty deciding whether to block command decoding,
> > or to decode and ignore the command if restrictions are enabled.
> >
> > The reason being that, in the qtest, the WRSR command code gets ignored, and
> > then the subsequent write data gets interpreted as some random command code.
> > We had elected to decode and ignore the command, but I think the
> > datasheet actually describes that the command won’t be decoded successfully,
> > so you’re probably right, we should put this logic in decode_new_cmd.
> >
> > Most likely, the q
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
Just find out how to use mutt to reply all in the thread.
repeat the previous comments. Add STATE_HIZ to handle decode_new_command
aborting gracefully.
On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
>
>
> > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> > wrote:
> >
> > Hi Iris,
> >
> > Looks good some, a couple of comments below.
> >
> > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> >> From: Iris Chen
> >>
> >> Signed-off-by: Iris Chen
> >> ---
> >> Addressed all comments from V1. The biggest change: removed
> >> object_class_property_add.
> >>
> >> hw/block/m25p80.c | 37 +++
> >> tests/qtest/aspeed_smc-test.c | 2 ++
> >> 2 files changed, 39 insertions(+)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 81ba3da4df..1a20bd55d4 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -27,12 +27,14 @@
> >> #include "hw/qdev-properties.h"
> >> #include "hw/qdev-properties-system.h"
> >> #include "hw/ssi/ssi.h"
> >> +#include "hw/irq.h"
> >> #include "migration/vmstate.h"
> >> #include "qemu/bitops.h"
> >> #include "qemu/log.h"
> >> #include "qemu/module.h"
> >> #include "qemu/error-report.h"
> >> #include "qapi/error.h"
> >> +#include "qapi/visitor.h"
> >> #include "trace.h"
> >> #include "qom/object.h"
> >>
> >> @@ -472,11 +474,13 @@ struct Flash {
> >> uint8_t spansion_cr2v;
> >> uint8_t spansion_cr3v;
> >> uint8_t spansion_cr4v;
> >> +bool wp_level;
> >> bool write_enable;
> >> bool four_bytes_address_mode;
> >> bool reset_enable;
> >> bool quad_enable;
> >> bool aai_enable;
> >> +bool status_register_write_disabled;
> >> uint8_t ear;
> >>
> >> int64_t dirty_page;
> >> @@ -723,6 +727,21 @@ static void complete_collecting_data(Flash *s)
> >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >> break;
> >> case WRSR:
> >> +/*
> >> + * If WP# is low and status_register_write_disabled is high,
> >> + * status register writes are disabled.
> >> + * This is also called "hardware protected mode" (HPM). All other
> >> + * combinations of the two states are called "software protected
> >> mode"
> >> + * (SPM), and status register writes are permitted.
> >> + */
> >> +if ((s->wp_level == 0 && s->status_register_write_disabled)
> >> +|| !s->write_enable) {
> >
> > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > command, otherwise the state machinery will not advance to this function
> > (meaning that above check for !s->write_enable will never hit as far as I
> > can
> > tell). A suggestion is to move the check for wp_level and
> > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
>
> Oh good catch! Yes actually, in our fork, we also removed the write_enable
> guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> or both checks in complete_collecting_data.
>
> I think we had some difficulty deciding whether to block command decoding,
> or to decode and ignore the command if restrictions are enabled.
>
> The reason being that, in the qtest, the WRSR command code gets ignored, and
> then the subsequent write data gets interpreted as some random command code.
> We had elected to decode and ignore the command, but I think the
> datasheet actually describes that the command won’t be decoded successfully,
> so you’re probably right, we should put this logic in decode_new_cmd.
>
> Most likely, the qtest will also need to be modified to reset the transfer
> state machine after a blocked write command. I can’t remember if
> exiting and re-entering user mode is sufficient for that, but something
> like that is probably possible.
>
> Thanks for catching this!
> Peter
>
I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore
following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following o
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
>
>
> > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> > wrote:
> >
> > Hi Iris,
> >
> > Looks good some, a couple of comments below.
> >
> > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> >> From: Iris Chen
> >>
> >> Signed-off-by: Iris Chen
> >> ---
> >> Addressed all comments from V1. The biggest change: removed
> >> object_class_property_add.
> >>
> >> hw/block/m25p80.c | 37 +++
> >> tests/qtest/aspeed_smc-test.c | 2 ++
> >> 2 files changed, 39 insertions(+)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 81ba3da4df..1a20bd55d4 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -27,12 +27,14 @@
> >> #include "hw/qdev-properties.h"
> >> #include "hw/qdev-properties-system.h"
> >> #include "hw/ssi/ssi.h"
> >> +#include "hw/irq.h"
> >> #include "migration/vmstate.h"
> >> #include "qemu/bitops.h"
> >> #include "qemu/log.h"
> >> #include "qemu/module.h"
> >> #include "qemu/error-report.h"
> >> #include "qapi/error.h"
> >> +#include "qapi/visitor.h"
> >> #include "trace.h"
> >> #include "qom/object.h"
> >>
> >> @@ -472,11 +474,13 @@ struct Flash {
> >> uint8_t spansion_cr2v;
> >> uint8_t spansion_cr3v;
> >> uint8_t spansion_cr4v;
> >> +bool wp_level;
> >> bool write_enable;
> >> bool four_bytes_address_mode;
> >> bool reset_enable;
> >> bool quad_enable;
> >> bool aai_enable;
> >> +bool status_register_write_disabled;
> >> uint8_t ear;
> >>
> >> int64_t dirty_page;
> >> @@ -723,6 +727,21 @@ static void complete_collecting_data(Flash *s)
> >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >> break;
> >> case WRSR:
> >> +/*
> >> + * If WP# is low and status_register_write_disabled is high,
> >> + * status register writes are disabled.
> >> + * This is also called "hardware protected mode" (HPM). All other
> >> + * combinations of the two states are called "software protected
> >> mode"
> >> + * (SPM), and status register writes are permitted.
> >> + */
> >> +if ((s->wp_level == 0 && s->status_register_write_disabled)
> >> +|| !s->write_enable) {
> >
> > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > command, otherwise the state machinery will not advance to this function
> > (meaning that above check for !s->write_enable will never hit as far as I
> > can
> > tell). A suggestion is to move the check for wp_level and
> > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
>
> Oh good catch! Yes actually, in our fork, we also removed the write_enable
> guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> or both checks in complete_collecting_data.
>
> I think we had some difficulty deciding whether to block command decoding,
> or to decode and ignore the command if restrictions are enabled.
>
> The reason being that, in the qtest, the WRSR command code gets ignored, and
> then the subsequent write data gets interpreted as some random command code.
> We had elected to decode and ignore the command, but I think the
> datasheet actually describes that the command won’t be decoded successfully,
> so you’re probably right, we should put this logic in decode_new_cmd.
>
> Most likely, the qtest will also need to be modified to reset the transfer
> state machine after a blocked write command. I can’t remember if
> exiting and re-entering user mode is sufficient for that, but something
> like that is probably possible.
>
> Thanks for catching this!
> Peter
>
I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following on byte when in this state.
This is to simulating the real device operation behavior
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
`
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
`
> >
> >> +qemu_log_mask(LOG_GUEST_ERROR,
> >> + "M25P80: Status register write is disabled!\n");
> >> +break;
> >> +}
> >> +s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> >> +
> >> switch (get_man(s)) {
> >> case MAN_SPANSION:
> >> s->quad_enable = !!(s->data[1] & 0x02);
> >> @@ -1195,6 +1214,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >>
> >> case RDSR:
> >> s->data[0] = (!!s->write_enable) << 1;
> >> +s->data[0] |= (!!s->status_register_write_disabled) << 7;
> >> +
> >> if (get_man(s) == MAN_MACRONIX ||
[Qemu-devel] Mouse cursor position is un-matched when I use Qemu in VNC mode.
Hi,
I installed WinXP in QEMU (with KVM enabled), now I used a VNC client
(AndroidVNC, which is modified from TightVNC client) to connect QEMU
via VNC mode. All works fine except for the mouse position is
un-matched.
When I move the mouse slowly, the mouse cursor in QEMU moves slower
than it moves in client.
When I move the mouse quickly, the mouse cursor in QEMU moves quicker
than it moves in client.
I tried to debug it.
The client (androidVNC) generates correct screen coordinates of mouse cursor.
The server (QEMU) also receives correct screen coordinates, then
calculate the difference between current position and last position,
then put events into ps2_queue. Then ps2_read_data reads these data.
the call stack is:
ps2_mouse_event
kbd_mouse_event
pointer_event
In the following code of function pointer_event, it runs into the last
else branch:
if (vs-absolute) {
kbd_mouse_event(ds_get_width(vs-ds) 1 ?
x * 0x7FFF / (ds_get_width(vs-ds) - 1) : 0x4000,
ds_get_height(vs-ds) 1 ?
y * 0x7FFF / (ds_get_height(vs-ds) - 1) : 0x4000,
dz, buttons);
} else if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE)) {
x -= 0x7FFF;
y -= 0x7FFF;
kbd_mouse_event(x, y, dz, buttons);
} else {
if (vs-last_x != -1)
kbd_mouse_event(x - vs-last_x,
y - vs-last_y,
dz, buttons);
vs-last_x = x;
vs-last_y = y;
}
vs-absolute seems initiated by a mouse_handler. Can I change it in somewhere?
The consumer of mouse data seems in ps2_read_data, then I tracked the
call hierarchy of it is:
#0 ps2_read_data (opaque=0x8aafa48)
#1 0x08197686 in kbd_read_data (opaque=0x8aaf7a0, addr=96)
#2 0x080bb19e in ioport_read (index=0, address=96) at ioport.c:68
#3 0x080bb9de in cpu_inb (addr=96) at ioport.c:284
#4 0x0806ebd3 in kvm_handle_io (port=96, data=0xb7438000, direction=0,
#5 0x0806f090 in kvm_cpu_exec (env=0x8991708)
I stopped at here and cannot continue debugging to see where the
cursor position becomes incorrect since this data structure (kvm_run)
is in linux/kvm.h:
switch (run-exit_reason) {
case KVM_EXIT_IO:
DPRINTF(handle_io\n);
ret = kvm_handle_io(run-io.port,
(uint8_t *)run + run-io.data_offset,
run-io.direction,
run-io.size,
run-io.count);
break;
I also did some experiments:
I tried androidVNC client to connect WinXP (with TightVNC server)
installed in VirtualBox. It works fine (at least better).
I also tried use Ubuntu's VNC client (vinagre) to connect QEMU-winxp,
it also works fine. I tried to debug it and found it runs into the
second branch (with this feature: VNC_FEATURE_POINTER_TYPE_CHANGE).
But this feature seems not supported in androidVNC client.
So anyone can give me some clues to solve this issue?
Thanks a lot!
--
Best Regards
Dan Zhang
