* Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step
From: Peter Maydell @ 2014-11-26 19:27 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-devel, arm-mail-list,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org,
Christoffer Dall, Marc Zyngier, Alexander Graf, J. Kiszka,
David Hildenbrand, Bharat Bhushan, bp-l3A5Bk7waGM, Paolo Bonzini,
Gleb Natapov, Russell King, Catalin Marinas, Will Deacon,
Lorenzo Pieralisi, open list, open list:ABI/API
In-Reply-To: <1416931805-23223-6-git-send-email-alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 25 November 2014 at 16:10, Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> This adds support for single-stepping the guest. As userspace can and
> will manipulate guest registers before restarting any tweaking of the
> registers has to occur just before control is passed back to the guest.
> Furthermore while guest debugging is in effect we need to squash the
> ability of the guest to single-step itself as we have no easy way of
> re-entering the guest after the exception has been delivered to the
> hypervisor.
A corner case I don't think this patch handles: if the debugger
tries to single step an insn which is emulated by the
hypervisor (because it's a load/store which is trapped and
handled as emulated mmio in userspace) then we won't
correctly update the single-step state machine (and so we'll end
up incorrectly stopping after the following insn rather than
before, I think).
You should be able to achieve this effect by simply always clearing
the guest's PSTATE.SS when you advance the PC to skip the emulated
instruction (cf the comment in the pseudocode SSAdvance() function).
I think we should also be doing this PC advance on return from
userspace's handling of the mmio rather than before we drop back
to userspace as we do now, but I can't remember why I think that.
Christoffer, I don't suppose you recall, do you? I think it was
you I had this conversation with on IRC a month or so back...
-- PMM
^ permalink raw reply
* Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Murali Karicheri @ 2014-11-26 18:29 UTC (permalink / raw)
To: Chunyan Zhang
Cc: mark.rutland, gnomes, heiko, broonie, catalin.marinas,
will.deacon, andrew, linux-api, jslaby, artagnon, lanqing.liu,
arnd, corbet, zhang.lyra, zhizhou.zhang, geng.ren,
linux-arm-kernel, linux-serial, grant.likely, orsonzhai,
florian.vaussard, devicetree, jason, pawel.moll, ijc+devicetree,
hytszk, rrichter, broonie, wei.qiao, sprdlinux, shawn.guo, gregkh,
linux-doc, linux-kernel, robh+dt, galak
In-Reply-To: <1416917818-10506-6-git-send-email-chunyan.zhang@spreadtrum.com>
On 11/25/2014 07:16 AM, Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
>
> Signed-off-by: Chunyan Zhang<chunyan.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai<orson.zhai@spreadtrum.com>
> Originally-by: Lanqing Liu<lanqing.liu@spreadtrum.com>
> ---
> Documentation/devices.txt | 3 +
> drivers/tty/serial/Kconfig | 23 ++
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/sprd_serial.c | 752 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> 5 files changed, 782 insertions(+)
> create mode 100644 drivers/tty/serial/sprd_serial.c
>
> diff --git a/Documentation/devices.txt b/Documentation/devices.txt
> index 87b4c5e..1da0432 100644
> --- a/Documentation/devices.txt
> +++ b/Documentation/devices.txt
> @@ -2816,6 +2816,9 @@ Your cooperation is appreciated.
> 210 = /dev/ttyMAX1 MAX3100 serial port 1
> 211 = /dev/ttyMAX2 MAX3100 serial port 2
> 212 = /dev/ttyMAX3 MAX3100 serial port 3
> + 213 = /dev/ttySPX0 SPRD serial port 0
> + ...
> + 216 = /dev/ttySPX3 SPRD serial port 3
>
> 205 char Low-density serial ports (alternate device)
> 0 = /dev/culu0 Callout device for ttyLU0
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 649b784..2c2cf60 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1573,6 +1573,29 @@ config SERIAL_MEN_Z135
> This driver can also be build as a module. If so, the module will be called
> men_z135_uart.ko
>
> +config SERIAL_SPRD
> + tristate "Support for SPRD serial"
> + depends on ARCH_SPRD
> + select SERIAL_CORE
> + help
> + This enables the driver for the Spreadtrum's serial.
> +
> +config SERIAL_SPRD_NR
> + int "Maximum number of sprd serial ports"
> + depends on SERIAL_SPRD
> + default "4"
> +
> +config SERIAL_SPRD_CONSOLE
> + bool "SPRD UART console support"
> + depends on SERIAL_SPRD=y
> + select SERIAL_CORE_CONSOLE
> + select SERIAL_EARLYCON
> + help
> + Support for early debug console using Spreadtrum's serial. This enables
> + the console before standard serial driver is probed. This is enabled
> + with "earlycon" on the kernel command line. The console is
> + enabled when early_param is processed.
> +
> endmenu
>
> config SERIAL_MCTRL_GPIO
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 9a548ac..4801aca 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC) += arc_uart.o
> obj-$(CONFIG_SERIAL_RP2) += rp2.o
> obj-$(CONFIG_SERIAL_FSL_LPUART) += fsl_lpuart.o
> obj-$(CONFIG_SERIAL_MEN_Z135) += men_z135_uart.o
> +obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
>
> # GPIOLIB helpers for modem control lines
> obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> new file mode 100644
> index 0000000..58214c8
> --- /dev/null
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -0,0 +1,752 @@
> +/*
> + * Copyright (C) 2012 Spreadtrum Communications Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include<linux/module.h>
> +#include<linux/tty.h>
> +#include<linux/ioport.h>
> +#include<linux/console.h>
> +#include<linux/platform_device.h>
> +#include<linux/tty_flip.h>
> +#include<linux/serial_core.h>
> +#include<linux/serial.h>
> +#include<linux/delay.h>
> +#include<linux/io.h>
> +#include<asm/irq.h>
> +#include<linux/slab.h>
> +#include<linux/of.h>
> +#include<linux/kernel.h>
> +#include<linux/slab.h>
> +#include<linux/clk.h>
How about sorting this includes? asm/irq.h go first followed linux/ in
alphabatical order?
> +
> +/* device name */
> +#define UART_NR_MAX CONFIG_SERIAL_SPRD_NR
> +#define SPRD_TTY_NAME "ttySPX"
> +#define SPRD_TTY_MAJOR 204
> +#define SPRD_TTY_MINOR_START 213
> +#define SPRD_FIFO_SIZE 128
> +#define SPRD_DEF_RATE 26000000
> +
> +/* the offset of serial registers and BITs for them */
> +/* data registers */
> +#define SPRD_TXD 0x0000
> +#define SPRD_RXD 0x0004
> +
> +/* line status register and its BITs */
> +#define SPRD_LSR 0x0008
> +#define SPRD_LSR_OE BIT(4)
> +#define SPRD_LSR_FE BIT(3)
> +#define SPRD_LSR_PE BIT(2)
> +#define SPRD_LSR_BI BIT(7)
> +#define SPRD_LSR_TX_OVER BIT(15)
> +
> +/* data number in TX and RX fifo */
> +#define SPRD_STS1 0x000C
> +
> +/* interrupt enable register and its BITs */
> +#define SPRD_IEN 0x0010
> +#define SPRD_IEN_RX_FULL BIT(0)
> +#define SPRD_IEN_TX_EMPTY BIT(1)
> +#define SPRD_IEN_BREAK_DETECT BIT(7)
> +#define SPRD_IEN_TIMEOUT BIT(13)
> +
> +/* interrupt clear register */
> +#define SPRD_ICLR 0x0014
> +
> +/* line control register */
> +#define SPRD_LCR 0x0018
> +#define SPRD_LCR_STOP_1BIT 0x10
> +#define SPRD_LCR_STOP_2BIT 0x30
> +#define SPRD_LCR_DATA_LEN (BIT(2) | BIT(3))
> +#define SPRD_LCR_DATA_LEN5 0x0
> +#define SPRD_LCR_DATA_LEN6 0x4
> +#define SPRD_LCR_DATA_LEN7 0x8
> +#define SPRD_LCR_DATA_LEN8 0xc
> +#define SPRD_LCR_PARITY (BIT(0) | BIT(1))
> +#define SPRD_LCR_PARITY_EN 0x2
> +#define SPRD_LCR_EVEN_PAR 0x0
> +#define SPRD_LCR_ODD_PAR 0x1
> +
> +/* control register 1 */
> +#define SPRD_CTL1 0x001C
> +#define RX_HW_FLOW_CTL_THLD BIT(6)
> +#define RX_HW_FLOW_CTL_EN BIT(7)
> +#define TX_HW_FLOW_CTL_EN BIT(8)
> +
> +/* fifo threshold register */
> +#define SPRD_CTL2 0x0020
> +#define THLD_TX_EMPTY 0x40
> +#define THLD_RX_FULL 0x40
> +
> +/* config baud rate register */
> +#define SPRD_CLKD0 0x0024
> +#define SPRD_CLKD1 0x0028
> +
> +/* interrupt mask status register */
> +#define SPRD_IMSR 0x002C
> +#define SPRD_IMSR_RX_FIFO_FULL BIT(0)
> +#define SPRD_IMSR_TX_FIFO_EMPTY BIT(1)
> +#define SPRD_IMSR_BREAK_DETECT BIT(7)
> +#define SPRD_IMSR_TIMEOUT BIT(13)
> +
> +struct reg_backup {
> + uint32_t ien;
> + uint32_t ctrl0;
> + uint32_t ctrl1;
> + uint32_t ctrl2;
> + uint32_t clkd0;
> + uint32_t clkd1;
> + uint32_t dspwait;
> +};
> +struct sprd_uart_port {
> + struct uart_port port;
> + struct reg_backup reg_bak;
> + char name[16];
> +};
> +static struct sprd_uart_port *sprd_port[UART_NR_MAX] = { NULL };
> +
> +static inline unsigned int serial_in(struct uart_port *port, int offset)
> +{
> + return readl_relaxed(port->membase + offset);
> +}
> +
> +static inline void serial_out(struct uart_port *port, int offset, int value)
> +{
> + writel_relaxed(value, port->membase + offset);
> +}
> +
> +static unsigned int sprd_tx_empty(struct uart_port *port)
> +{
> + if (serial_in(port, SPRD_STS1)& 0xff00)
> + return 0;
> + else
> + return TIOCSER_TEMT;
> +}
> +
> +static unsigned int sprd_get_mctrl(struct uart_port *port)
> +{
> + return TIOCM_DSR | TIOCM_CTS;
> +}
> +
> +static void sprd_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> + /* nothing to do */
> +}
> +
> +static void sprd_stop_tx(struct uart_port *port)
> +{
> + unsigned int ien, iclr;
> +
> + iclr = serial_in(port, SPRD_ICLR);
> + ien = serial_in(port, SPRD_IEN);
> +
> + iclr |= SPRD_IEN_TX_EMPTY;
> + ien&= ~SPRD_IEN_TX_EMPTY;
> +
> + serial_out(port, SPRD_ICLR, iclr);
> + serial_out(port, SPRD_IEN, ien);
> +}
> +
> +static void sprd_start_tx(struct uart_port *port)
> +{
> + unsigned int ien;
> +
> + ien = serial_in(port, SPRD_IEN);
> + if (!(ien& SPRD_IEN_TX_EMPTY)) {
> + ien |= SPRD_IEN_TX_EMPTY;
> + serial_out(port, SPRD_IEN, ien);
> + }
> +}
> +
> +static void sprd_stop_rx(struct uart_port *port)
> +{
> + unsigned int ien, iclr;
> +
> + iclr = serial_in(port, SPRD_ICLR);
> + ien = serial_in(port, SPRD_IEN);
> +
> + ien&= ~(SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT);
> + iclr |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT;
> +
> + serial_out(port, SPRD_IEN, ien);
> + serial_out(port, SPRD_ICLR, iclr);
> +}
> +
> +/* The Sprd serial does not support this function. */
> +static void sprd_break_ctl(struct uart_port *port, int break_state)
> +{
> + /* nothing to do */
> +}
> +
> +static inline int handle_lsr_errors(struct uart_port *port,
> + unsigned int *flag, unsigned int *lsr)
> +{
> + int ret = 0;
> +
> + /* stastics */
> + if (*lsr& SPRD_LSR_BI) {
> + *lsr&= ~(SPRD_LSR_FE | SPRD_LSR_PE);
> + port->icount.brk++;
> + ret = uart_handle_break(port);
> + if (ret)
> + return ret;
> + } else if (*lsr& SPRD_LSR_PE)
> + port->icount.parity++;
> + else if (*lsr& SPRD_LSR_FE)
> + port->icount.frame++;
> + if (*lsr& SPRD_LSR_OE)
> + port->icount.overrun++;
> +
> + /* mask off conditions which should be ignored */
> + *lsr&= port->read_status_mask;
> + if (*lsr& SPRD_LSR_BI)
> + *flag = TTY_BREAK;
> + else if (*lsr& SPRD_LSR_PE)
> + *flag = TTY_PARITY;
> + else if (*lsr& SPRD_LSR_FE)
> + *flag = TTY_FRAME;
> +
> + return ret;
> +}
> +
> +static inline void sprd_rx(int irq, void *dev_id)
> +{
> + struct uart_port *port = (struct uart_port *)dev_id;
> + struct tty_port *tty =&port->state->port;
> + unsigned int ch, flag, lsr, max_count = 2048;
> +
> + while ((serial_in(port, SPRD_STS1)& 0x00ff)&& max_count--) {
> + lsr = serial_in(port, SPRD_LSR);
> + ch = serial_in(port, SPRD_RXD);
> + flag = TTY_NORMAL;
> + port->icount.rx++;
> +
> + if (unlikely(lsr& (SPRD_LSR_BI | SPRD_LSR_PE
> + | SPRD_LSR_FE | SPRD_LSR_OE)))
> + if (handle_lsr_errors(port,&lsr,&flag))
> + continue;
> + if (uart_handle_sysrq_char(port, ch))
> + continue;
> +
> + uart_insert_char(port, lsr, SPRD_LSR_OE, ch, flag);
> + }
> +
> + tty_flip_buffer_push(tty);
> +}
> +
> +static inline void sprd_tx(int irq, void *dev_id)
> +{
> + struct uart_port *port = dev_id;
> + struct circ_buf *xmit =&port->state->xmit;
> + int count;
> +
> + if (port->x_char) {
> + serial_out(port, SPRD_TXD, port->x_char);
> + port->icount.tx++;
> + port->x_char = 0;
> + return;
> + }
> + if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
> + sprd_stop_tx(port);
> + return;
> + }
> + count = THLD_TX_EMPTY;
> + do {
> + serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
> + xmit->tail = (xmit->tail + 1)& (UART_XMIT_SIZE - 1);
> + port->icount.tx++;
> + if (uart_circ_empty(xmit))
> + break;
> + } while (--count> 0);
> +
> + if (uart_circ_chars_pending(xmit)< WAKEUP_CHARS)
> + uart_write_wakeup(port);
> +
> + if (uart_circ_empty(xmit))
> + sprd_stop_tx(port);
> +}
> +
> +/*
> + *this handles the interrupt from one port
> + */
> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
> +{
> + struct uart_port *port = (struct uart_port *)dev_id;
> + u32 ims;
> +
> + ims = serial_in(port, SPRD_IMSR);
> +
> + serial_out(port, SPRD_ICLR, ~0);
> +
> + if (ims& (SPRD_IMSR_RX_FIFO_FULL |
> + SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT)) {
> + sprd_rx(irq, port);
> + }
> + if (ims& SPRD_IMSR_TX_FIFO_EMPTY)
> + sprd_tx(irq, port);
> +
> + return IRQ_HANDLED;
You are always returning IRQ_HANDLED and this is registered as a SHARED
irq. Is there a chance this handler is called and the irq event doesn't
belong to this device?
Murali
> +}
> +
> +static int sprd_startup(struct uart_port *port)
> +{
> + int ret = 0;
> + unsigned int ien, ctrl1;
> + struct sprd_uart_port *sp;
> +
> + serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY<< 8) | THLD_RX_FULL));
> +
> + /* clear rx fifo */
> + while (serial_in(port, SPRD_STS1)& 0x00ff)
> + serial_in(port, SPRD_RXD);
> +
> + /* clear tx fifo */
> + while (serial_in(port, SPRD_STS1)& 0xff00)
> + ;
> +
> + /* clear interrupt */
> + serial_out(port, SPRD_IEN, 0x0);
> + serial_out(port, SPRD_ICLR, ~0);
> +
> + /* allocate irq */
> + sp = container_of(port, struct sprd_uart_port, port);
> + snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
> + ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
> + IRQF_SHARED, sp->name, port);
> + if (ret) {
> + dev_err(port->dev, "fail to request serial irq %d\n",
> + port->irq);
> + return ret;
> + }
> + ctrl1 = serial_in(port, SPRD_CTL1);
> + ctrl1 |= 0x3e00 | THLD_RX_FULL;
> + serial_out(port, SPRD_CTL1, ctrl1);
> +
> + /* enable interrupt */
> + spin_lock(&port->lock);
> + ien = serial_in(port, SPRD_IEN);
> + ien |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT | SPRD_IEN_TIMEOUT;
> + serial_out(port, SPRD_IEN, ien);
> + spin_unlock(&port->lock);
> +
> + return 0;
> +}
> +
> +static void sprd_shutdown(struct uart_port *port)
> +{
> + serial_out(port, SPRD_IEN, 0x0);
> + serial_out(port, SPRD_ICLR, ~0);
> + devm_free_irq(port->dev, port->irq, port);
> +}
> +
> +static void sprd_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *old)
> +{
> + unsigned int baud, quot;
> + unsigned int lcr, fc;
> +
> + /* ask the core to calculate the divisor for us */
> + baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
> +
> + quot = (unsigned int)((port->uartclk + baud / 2) / baud);
> +
> + /* set data length */
> + lcr = serial_in(port, SPRD_LCR);
> + lcr&= ~SPRD_LCR_DATA_LEN;
> + switch (termios->c_cflag& CSIZE) {
> + case CS5:
> + lcr |= SPRD_LCR_DATA_LEN5;
> + break;
> + case CS6:
> + lcr |= SPRD_LCR_DATA_LEN6;
> + break;
> + case CS7:
> + lcr |= SPRD_LCR_DATA_LEN7;
> + break;
> + case CS8:
> + default:
> + lcr |= SPRD_LCR_DATA_LEN8;
> + break;
> + }
> +
> + /* calculate stop bits */
> + lcr&= ~(SPRD_LCR_STOP_1BIT | SPRD_LCR_STOP_2BIT);
> + if (termios->c_cflag& CSTOPB)
> + lcr |= SPRD_LCR_STOP_2BIT;
> + else
> + lcr |= SPRD_LCR_STOP_1BIT;
> +
> + /* calculate parity */
> + lcr&= ~SPRD_LCR_PARITY;
> + if (termios->c_cflag& PARENB) {
> + lcr |= SPRD_LCR_PARITY_EN;
> + if (termios->c_cflag& PARODD)
> + lcr |= SPRD_LCR_ODD_PAR;
> + else
> + lcr |= SPRD_LCR_EVEN_PAR;
> + }
> +
> + /* change the port state. */
> + /* update the per-port timeout */
> + uart_update_timeout(port, termios->c_cflag, baud);
> +
> + port->read_status_mask = SPRD_LSR_OE;
> + if (termios->c_iflag& INPCK)
> + port->read_status_mask |= SPRD_LSR_FE | SPRD_LSR_PE;
> + if (termios->c_iflag& (BRKINT | PARMRK))
> + port->read_status_mask |= SPRD_LSR_BI;
> +
> + /* characters to ignore */
> + port->ignore_status_mask = 0;
> + if (termios->c_iflag& IGNPAR)
> + port->ignore_status_mask |= SPRD_LSR_PE | SPRD_LSR_FE;
> + if (termios->c_iflag& IGNBRK) {
> + port->ignore_status_mask |= SPRD_LSR_BI;
> + /*
> + * If we're ignoring parity and break indicators,
> + * ignore overruns too (for real raw support).
> + */
> + if (termios->c_iflag& IGNPAR)
> + port->ignore_status_mask |= SPRD_LSR_OE;
> + }
> +
> + /* flow control */
> + fc = serial_in(port, SPRD_CTL1);
> + fc&= ~(RX_HW_FLOW_CTL_THLD | RX_HW_FLOW_CTL_EN | TX_HW_FLOW_CTL_EN);
> + if (termios->c_cflag& CRTSCTS) {
> + fc |= RX_HW_FLOW_CTL_THLD;
> + fc |= RX_HW_FLOW_CTL_EN;
> + fc |= TX_HW_FLOW_CTL_EN;
> + }
> +
> + /* clock divider bit0~bit15 */
> + serial_out(port, SPRD_CLKD0, quot& 0xffff);
> +
> + /* clock divider bit16~bit20 */
> + serial_out(port, SPRD_CLKD1, (quot& 0x1f0000)>> 16);
> + serial_out(port, SPRD_LCR, lcr);
> + fc |= 0x3e00 | THLD_RX_FULL;
> + serial_out(port, SPRD_CTL1, fc);
> +}
> +
> +static const char *sprd_type(struct uart_port *port)
> +{
> + return "SPX";
> +}
> +
> +static void sprd_release_port(struct uart_port *port)
> +{
> + /* nothing to do */
> +}
> +
> +static int sprd_request_port(struct uart_port *port)
> +{
> + return 0;
> +}
> +
> +static void sprd_config_port(struct uart_port *port, int flags)
> +{
> + if (flags& UART_CONFIG_TYPE)
> + port->type = PORT_SPRD;
> +}
> +
> +static int sprd_verify_port(struct uart_port *port,
> + struct serial_struct *ser)
> +{
> + if (unlikely(ser->type != PORT_SPRD))
> + return -EINVAL;
> + if (unlikely(port->irq != ser->irq))
> + return -EINVAL;
> + return 0;
> +}
> +
> +static struct uart_ops serial_sprd_ops = {
> + .tx_empty = sprd_tx_empty,
> + .get_mctrl = sprd_get_mctrl,
> + .set_mctrl = sprd_set_mctrl,
> + .stop_tx = sprd_stop_tx,
> + .start_tx = sprd_start_tx,
> + .stop_rx = sprd_stop_rx,
> + .break_ctl = sprd_break_ctl,
> + .startup = sprd_startup,
> + .shutdown = sprd_shutdown,
> + .set_termios = sprd_set_termios,
> + .type = sprd_type,
> + .release_port = sprd_release_port,
> + .request_port = sprd_request_port,
> + .config_port = sprd_config_port,
> + .verify_port = sprd_verify_port,
> +};
> +
> +#ifdef CONFIG_SERIAL_SPRD_CONSOLE
> +static inline void wait_for_xmitr(struct uart_port *port)
> +{
> + unsigned int status, tmout = 10000;
> +
> + /* wait up to 10ms for the character(s) to be sent */
> + do {
> + status = serial_in(port, SPRD_STS1);
> + if (--tmout == 0)
> + break;
> + udelay(1);
> + } while (status& 0xff00);
> +}
> +
> +static void sprd_console_putchar(struct uart_port *port, int ch)
> +{
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_TXD, ch);
> +}
> +
> +static void sprd_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *port = (struct uart_port *)sprd_port[co->index];
> + int ien;
> + int locked = 1;
> +
> + if (oops_in_progress)
> + locked = spin_trylock(&port->lock);
> + else
> + spin_lock(&port->lock);
> + /* save the IEN then disable the interrupts */
> + ien = serial_in(port, SPRD_IEN);
> + serial_out(port, SPRD_IEN, 0x0);
> +
> + uart_console_write(port, s, count, sprd_console_putchar);
> +
> + /* wait for transmitter to become empty and restore the IEN */
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_IEN, ien);
> + if (locked)
> + spin_unlock(&port->lock);
> +}
> +
> +static int __init sprd_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *port;
> + int baud = 115200;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (unlikely(co->index>= UART_NR_MAX || co->index< 0))
> + co->index = 0;
> +
> + port = (struct uart_port *)sprd_port[co->index];
> + if (port == NULL) {
> + pr_info("srial port %d not yet initialized\n", co->index);
> + return -ENODEV;
> + }
> + if (options)
> + uart_parse_options(options,&baud,&parity,&bits,&flow);
> +
> + return uart_set_options(port, co, baud, parity, bits, flow);
> +}
> +
> +static struct uart_driver sprd_uart_driver;
> +static struct console sprd_console = {
> + .name = SPRD_TTY_NAME,
> + .write = sprd_console_write,
> + .device = uart_console_device,
> + .setup = sprd_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data =&sprd_uart_driver,
> +};
> +
> +#define SPRD_CONSOLE (&sprd_console)
> +
> +/* Support for earlycon */
> +static void sprd_putc(struct uart_port *port, int c)
> +{
> + while (!(readl(port->membase + SPRD_LSR)& SPRD_LSR_TX_OVER))
> + ;
> + writeb(c, port->membase + SPRD_TXD);
> +}
> +
> +static void sprd_early_write(struct console *con, const char *s,
> + unsigned n)
> +{
> + struct earlycon_device *dev = con->data;
> +
> + uart_console_write(&dev->port, s, n, sprd_putc);
> +}
> +
> +static int __init sprd_early_console_setup(
> + struct earlycon_device *device,
> + const char *opt)
> +{
> + if (!device->port.membase)
> + return -ENODEV;
> +
> + device->con->write = sprd_early_write;
> + return 0;
> +}
> +
> +EARLYCON_DECLARE(sprd_serial, sprd_early_console_setup);
> +OF_EARLYCON_DECLARE(sprd_serial, "sprd,sc9836-uart",
> + sprd_early_console_setup);
> +
> +#else /* !CONFIG_SERIAL_SPRD_CONSOLE */
> +#define SPRD_CONSOLE NULL
> +#endif
> +
> +static struct uart_driver sprd_uart_driver = {
> + .owner = THIS_MODULE,
> + .driver_name = "sprd_serial",
> + .dev_name = SPRD_TTY_NAME,
> + .major = SPRD_TTY_MAJOR,
> + .minor = SPRD_TTY_MINOR_START,
> + .nr = UART_NR_MAX,
> + .cons = SPRD_CONSOLE,
> +};
> +
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *mem;
> + struct device_node *np = pdev->dev.of_node;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> +
> +
> + if (np)
> + pdev->id = of_alias_get_id(np, "serial");
> +
> + if (unlikely(pdev->id< 0 || pdev->id>= UART_NR_MAX)) {
> + dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
> + return -ENXIO;
> + }
> +
> + sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
> + if (!sprd_port[pdev->id])
> + return -ENOMEM;
> +
> + up = (struct uart_port *)sprd_port[pdev->id];
> + up->dev =&pdev->dev;
> + up->line = pdev->id;
> + up->type = PORT_SPRD;
> + up->iotype = SERIAL_IO_PORT;
> + up->uartclk = SPRD_DEF_RATE;
> + up->fifosize = SPRD_FIFO_SIZE;
> + up->ops =&serial_sprd_ops;
> + up->flags = ASYNC_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (unlikely(!mem)) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
> + up->mapbase = mem->start;
> + up->membase = ioremap(mem->start, resource_size(mem));
> +
> + irq = platform_get_irq(pdev, 0);
> + if (unlikely(irq< 0)) {
> + dev_err(&pdev->dev, "not provide irq resource\n");
> + return -ENODEV;
> + }
> + up->irq = irq;
> +
> + platform_set_drvdata(pdev, up);
> +
> + return uart_add_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_remove(struct platform_device *dev)
> +{
> + struct uart_port *up = platform_get_drvdata(dev);
> +
> + return uart_remove_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
> + struct reg_backup *reg_bak =&(sprd_port[id]->reg_bak);
> +
> + reg_bak->ien = serial_in(port, SPRD_IEN);
> + reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
> + reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
> + reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
> + reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
> + reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
> +
> + return 0;
> +}
> +
> +static int sprd_resume(struct platform_device *dev)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
> + struct reg_backup *reg_bak =&(sprd_port[id]->reg_bak);
> +
> + serial_out(port, SPRD_LCR, reg_bak->ctrl0);
> + serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
> + serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
> + serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
> + serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
> + serial_out(port, SPRD_IEN, reg_bak->ien);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id serial_ids[] = {
> + {.compatible = "sprd,sc9836-uart",},
> + {}
> +};
> +
> +static struct platform_driver sprd_platform_driver = {
> + .probe = sprd_probe,
> + .remove = sprd_remove,
> + .suspend = sprd_suspend,
> + .resume = sprd_resume,
> + .driver = {
> + .name = "sprd_serial",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(serial_ids),
> + },
> +};
> +
> +static int __init sprd_serial_init(void)
> +{
> + int ret = 0;
> +
> + ret = uart_register_driver(&sprd_uart_driver);
> + if (unlikely(ret != 0))
> + return ret;
> +
> + ret = platform_driver_register(&sprd_platform_driver);
> + if (unlikely(ret != 0))
> + uart_unregister_driver(&sprd_uart_driver);
> +
> + return ret;
> +}
> +
> +static void __exit sprd_serial_exit(void)
> +{
> + platform_driver_unregister(&sprd_platform_driver);
> + uart_unregister_driver(&sprd_uart_driver);
> +}
> +
> +module_init(sprd_serial_init);
> +module_exit(sprd_serial_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 16ad852..d9a8c88 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -247,4 +247,7 @@
> /* MESON */
> #define PORT_MESON 109
>
> +/* SPRD SERIAL */
> +#define PORT_SPRD 110
> +
> #endif /* _UAPILINUX_SERIAL_CORE_H */
--
Murali Karicheri
Linux Kernel, Texas Instruments
^ permalink raw reply
* Re: [tpmdd-devel] [PATCH v7 09/10] tpm: TPM 2.0 FIFO Interface
From: Jarkko Sakkinen @ 2014-11-26 18:10 UTC (permalink / raw)
To: Stefan Berger
Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, christophe.ricard,
josh.triplett, linux-api, linux-kernel, Will Arthur, tpmdd-devel,
jason.gunthorpe, trousers-tech
In-Reply-To: <5474FA07.6060204@linux.vnet.ibm.com>
On Tue, Nov 25, 2014 at 04:52:07PM -0500, Stefan Berger wrote:
> On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> >From: Will Arthur <will.c.arthur@intel.com>
> >
> >Detect TPM 2.0 by using the extended STS (STS3) register. For TPM 2.0,
> >instead of calling tpm_get_timeouts(), assign duration and timeout
> >values defined in the TPM 2.0 PTP specification.
> >
> >Signed-off-by: Will Arthur <will.c.arthur@intel.com>
> >Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >---
> > drivers/char/tpm/tpm_tis.c | 71 ++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 56 insertions(+), 15 deletions(-)
> >
> >diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> >index 7a2c59b..0b3c089 100644
> >--- a/drivers/char/tpm/tpm_tis.c
> >+++ b/drivers/char/tpm/tpm_tis.c
> >@@ -1,5 +1,6 @@
> > /*
> > * Copyright (C) 2005, 2006 IBM Corporation
> >+ * Copyright (C) 2014 Intel Corporation
> > *
> > * Authors:
> > * Leendert van Doorn <leendert@watson.ibm.com>
> >@@ -44,6 +45,10 @@ enum tis_status {
> > TPM_STS_DATA_EXPECT = 0x08,
> > };
> >
> >+enum tis_status3 {
> >+ TPM_STS3_TPM2_FAM = 0x04,
> >+};
> >+
> > enum tis_int_flags {
> > TPM_GLOBAL_INT_ENABLE = 0x80000000,
> > TPM_INTF_BURST_COUNT_STATIC = 0x100,
> >@@ -70,6 +75,7 @@ enum tis_defaults {
> > #define TPM_INT_STATUS(l) (0x0010 | ((l) << 12))
> > #define TPM_INTF_CAPS(l) (0x0014 | ((l) << 12))
> > #define TPM_STS(l) (0x0018 | ((l) << 12))
> >+#define TPM_STS3(l) (0x001b | ((l) << 12))
> > #define TPM_DATA_FIFO(l) (0x0024 | ((l) << 12))
> >
> > #define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
> >@@ -344,6 +350,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > {
> > int rc;
> > u32 ordinal;
> >+ unsigned long dur;
> >
> > rc = tpm_tis_send_data(chip, buf, len);
> > if (rc < 0)
> >@@ -355,9 +362,14 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> >
> > if (chip->vendor.irq) {
> > ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
> >+
> >+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >+ dur = tpm_calc_ordinal_duration(chip, ordinal);
> >+ else
> >+ dur = tpm_calc_ordinal_duration(chip, ordinal);
> >+
>
> Is this right? Shouldn't you call tpm2_calc_ordinal_duration for TPM2? If
> not don't check the flag but leave comment for why the same function can be
> called for both TPM versions.
No it's not right, it's a mistake. Thanks for noticing this :)
> > if (wait_for_tpm_stat
> >- (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >- tpm_calc_ordinal_duration(chip, ordinal),
> >+ (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
> > &chip->vendor.read_queue, false) < 0) {
> > rc = -ETIME;
> > goto out_err;
> >@@ -543,6 +555,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> > u32 vendor, intfcaps, intmask;
> > int rc, i, irq_s, irq_e, probe;
> > struct tpm_chip *chip;
> >+ u8 sts3;
> >
> > chip = tpmm_chip_alloc(dev, &tpm_tis);
> > if (IS_ERR(chip))
> >@@ -554,11 +567,28 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> > if (!chip->vendor.iobase)
> > return -EIO;
> >
> >+ sts3 = ioread8(chip->vendor.iobase + TPM_STS3(1));
> >+ if (sts3 & TPM_STS3_TPM2_FAM)
> >+ chip->flags = TPM_CHIP_FLAG_TPM2;
> >+
> > /* Default timeouts */
> >- chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> >- chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> >- chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> >- chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> >+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> >+ chip->vendor.timeout_a = usecs_to_jiffies(TPM2_TIMEOUT_A);
> >+ chip->vendor.timeout_b = usecs_to_jiffies(TPM2_TIMEOUT_B);
> >+ chip->vendor.timeout_c = usecs_to_jiffies(TPM2_TIMEOUT_C);
> >+ chip->vendor.timeout_d = usecs_to_jiffies(TPM2_TIMEOUT_D);
> >+ chip->vendor.duration[TPM_SHORT] =
> >+ usecs_to_jiffies(TPM2_DURATION_SHORT);
> >+ chip->vendor.duration[TPM_MEDIUM] =
> >+ usecs_to_jiffies(TPM2_DURATION_MEDIUM);
> >+ chip->vendor.duration[TPM_LONG] =
> >+ usecs_to_jiffies(TPM2_DURATION_LONG);
> >+ } else {
> >+ chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> >+ chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> >+ chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> >+ chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> >+ }
> >
> > if (wait_startup(chip, 0) != 0) {
> > rc = -ENODEV;
> >@@ -573,8 +603,8 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> > vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
> > chip->vendor.manufacturer_id = vendor;
> >
> >- dev_info(dev,
> >- "1.2 TPM (device-id 0x%X, rev-id %d)\n",
> >+ dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
> >+ (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
> > vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
> >
> > if (!itpm) {
> >@@ -616,13 +646,17 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> > dev_dbg(dev, "\tData Avail Int Support\n");
> >
> > /* get the timeouts before testing for irqs */
> >- if (tpm_get_timeouts(chip)) {
> >+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2) && tpm_get_timeouts(chip)) {
> > dev_err(dev, "Could not get TPM timeouts and durations\n");
> > rc = -ENODEV;
> > goto out_err;
> > }
> >
> >- if (tpm_do_selftest(chip)) {
> >+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >+ rc = tpm2_do_selftest(chip);
> >+ else
> >+ rc = tpm_do_selftest(chip);
> >+ if (rc) {
> > dev_err(dev, "TPM self test failed\n");
> > rc = -ENODEV;
> > goto out_err;
> >@@ -683,7 +717,10 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> > chip->vendor.probed_irq = 0;
> >
> > /* Generate Interrupts */
> >- tpm_gen_interrupt(chip);
> >+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >+ tpm2_gen_interrupt(chip);
> >+ else
> >+ tpm_gen_interrupt(chip);
> >
> > chip->vendor.irq = chip->vendor.probed_irq;
> >
> >@@ -759,14 +796,18 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
> > static int tpm_tis_resume(struct device *dev)
> > {
> > struct tpm_chip *chip = dev_get_drvdata(dev);
> >- int ret;
> >+ int ret = 0;
> >
> > if (chip->vendor.irq)
> > tpm_tis_reenable_interrupts(chip);
> >
> >- ret = tpm_pm_resume(dev);
> >- if (!ret)
> >- tpm_do_selftest(chip);
> >+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >+ tpm2_do_selftest(chip);
> >+ else {
> >+ ret = tpm_pm_resume(dev);
> >+ if (!ret)
> >+ tpm_do_selftest(chip);
> >+ }
>
> Only a self test necessary for TPM2 if resuming from ACPI sleep?
AFAIK firmware/BIOS should do TPM2_Startup according to the rules in the
section 12.2.3.2 of the Architecture specification.
> Other parts look good to me.
> Stefan
/Jarkko
^ permalink raw reply
* Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step
From: Alex Bennée @ 2014-11-26 18:00 UTC (permalink / raw)
To: Andrew Jones
Cc: kvm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A, marc.zyngier-5wv7dgnIgG8,
peter.maydell-QSEj5FYQhm4dnm+yROfE0A, agraf-l3A5Bk7waGM,
Lorenzo Pieralisi, Russell King, Gleb Natapov,
jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ, Will Deacon, open list,
open list:ABI/API, dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
Catalin Marinas, r65777-KZfg59tc24xl57MIdRCFDg,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, bp-l3A5Bk7waGM
In-Reply-To: <20141126164057.GE3245-EoAxxbxdFnFvD/m4c++uL6fLeoKvNuZc@public.gmane.org>
Andrew Jones <drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
>> This adds support for single-stepping the guest. As userspace can and
>> will manipulate guest registers before restarting any tweaking of the
>> registers has to occur just before control is passed back to the guest.
>> Furthermore while guest debugging is in effect we need to squash the
>> ability of the guest to single-step itself as we have no easy way of
>> re-entering the guest after the exception has been delivered to the
>> hypervisor.
>>
>> Signed-off-by: Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 48d26bb..a76daae 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -38,6 +38,7 @@
>> #include <asm/tlbflush.h>
>> #include <asm/cacheflush.h>
>> #include <asm/virt.h>
>> +#include <asm/debug-monitors.h>
>> #include <asm/kvm_arm.h>
>> #include <asm/kvm_asm.h>
>> #include <asm/kvm_mmu.h>
>> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> kvm_arm_set_running_vcpu(NULL);
>> }
>>
>> +/**
>> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
>> + * @kvm: pointer to the KVM struct
>> + * @kvm_guest_debug: the ioctl data buffer
>> + *
>> + * This sets up the VM for guest debugging. Care has to be taken when
>> + * manipulating guest registers as these will be set/cleared by the
>> + * hyper-visor controller, typically before each kvm_run event. As a
>> + * result modification of the guest registers needs to take place
>> + * after they have been restored in the hyp.S trampoline code.
>> + */
>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>> struct kvm_guest_debug *dbg)
>> {
>> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>
>> /* Single Step */
>> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> - kvm_info("SS requested, not yet implemented\n");
>> - return -EINVAL;
>> + kvm_info("SS requested\n");
>> + route_el2 = true;
>> }
>>
>> /* Software Break Points */
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 8da1043..78e5ae1 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -121,6 +121,7 @@ int main(void)
>> DEFINE(VCPU_FAR_EL2, offsetof(struct kvm_vcpu, arch.fault.far_el2));
>> DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>> DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags));
>> + DEFINE(GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
>> DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2));
>> DEFINE(VCPU_MDCR_EL2, offsetof(struct kvm_vcpu, arch.mdcr_el2));
>> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines));
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 28dc92b..6def054 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> return 0;
>> }
>>
>> +/**
>> + * kvm_handle_ss - handle single step exceptions
>> + *
>> + * @vcpu: the vcpu pointer
>
> same @run comment as other handler header in previous patch
Yeah I think I'll be merging them all together given the comments about
passing syndrome info directly.
>> + *
>> + * See: ARM ARM D2.12 for the details. While the host is routing debug
>> + * exceptions to it's handlers we have to suppress the ability of the
>> + * guest to trigger exceptions.
>> + */
>> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> + WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
>
> I'm not sure about this WARN_ON. Is there some scenario you were
> thinking of when you put it here? Is there some scenario where this
> could trigger so frequently we kill the log buffer?
The main one I had in mind was not suppressing the guest's attempt to
step while guest debugging was running.
<snip>
>>
>> -/* for KVM_SET_GUEST_DEBUG */
>> -
>> -#define KVM_GUESTDBG_ENABLE 0x00000001
>> -#define KVM_GUESTDBG_SINGLESTEP 0x00000002
>> -
>> struct kvm_guest_debug {
>> __u32 control;
>> __u32 pad;
>> @@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
>> __u16 padding[3];
>> };
>>
>> +#endif /* __ASSEMBLY__ */
>> +
>> +/* for KVM_SET_GUEST_DEBUG */
>> +
>> +#define KVM_GUESTDBG_ENABLE_SHIFT 0
>> +#define KVM_GUESTDBG_ENABLE (1 << KVM_GUESTDBG_ENABLE_SHIFT)
>> +#define KVM_GUESTDBG_SINGLESTEP_SHIFT 1
>> +#define KVM_GUESTDBG_SINGLESTEP (1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)
>
> EALIGN: we can tab these defines up better
Sure, I'll clean those up.
--
Alex Bennée
^ permalink raw reply
* Re: [PATCH 7/7] KVM: arm64: guest debug, HW assisted debug support
From: Andrew Jones @ 2014-11-26 17:34 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A, marc.zyngier-5wv7dgnIgG8,
peter.maydell-QSEj5FYQhm4dnm+yROfE0A, agraf-l3A5Bk7waGM,
Lorenzo Pieralisi, Russell King, Jonathan Corbet, Gleb Natapov,
jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ, AKASHI Takahiro,
open list:DOCUMENTATION, Will Deacon, open list,
open list:ABI/API, dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
Catalin Marinas, Srivatsa S. Bhat, r65777-KZfg59tc24xl57MIdRCFDg,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, bp-l3A5Bk7waGM
In-Reply-To: <1416931805-23223-8-git-send-email-alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Tue, Nov 25, 2014 at 04:10:05PM +0000, Alex Bennée wrote:
> This adds support for userspace to control the HW debug registers for
> guest debug. We'll only copy the $ARCH defined number across as that's
> all that hyp.S will use anyway. I've moved some helper functions into
> the hw_breakpoint.h header for re-use.
>
> As with single step we need to tweak the guest registers to enable the
> exceptions but we don't want to overwrite the guest copy of these
> registers so this is done close to the guest entry.
>
> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
> userspace to query the number of hardware break and watch points
> available on the host hardware.
>
> Signed-off-by: Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 9383359..5e8c673 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2593,7 +2593,7 @@ The top 16 bits of the control field are architecture specific control
> flags which can include the following:
>
> - KVM_GUESTDBG_USE_SW_BP: using software breakpoints [x86, arm64]
> - - KVM_GUESTDBG_USE_HW_BP: using hardware breakpoints [x86, s390]
> + - KVM_GUESTDBG_USE_HW_BP: using hardware breakpoints [x86, s390, arm64]
> - KVM_GUESTDBG_INJECT_DB: inject DB type exception [x86]
> - KVM_GUESTDBG_INJECT_BP: inject BP type exception [x86]
> - KVM_GUESTDBG_EXIT_PENDING: trigger an immediate guest exit [s390]
> @@ -2606,7 +2606,10 @@ we need to ensure the guest vCPUs architecture specific registers are
> updated to the correct (supplied) values.
>
> The second part of the structure is architecture specific and
> -typically contains a set of debug registers.
> +typically contains a set of debug registers. For arm64 the number of
> +debug registers is implementation defined and can be determined by
> +querying the KVM_CAP_GUEST_DEBUG_HW_BPS and KVM_CAP_GUEST_DEBUG_HW_WPS
> +capabilities.
>
> When debug events exit the main run loop with the reason
> KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a76daae..c8ec23a 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -39,6 +39,7 @@
> #include <asm/cacheflush.h>
> #include <asm/virt.h>
> #include <asm/debug-monitors.h>
> +#include <asm/hw_breakpoint.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_asm.h>
> #include <asm/kvm_mmu.h>
> @@ -341,8 +342,37 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>
> /* Hardware assisted Break and Watch points */
> if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> - kvm_info("HW BP support requested, not yet implemented\n");
> - return -EINVAL;
> + int i;
> + int nb = get_num_brps();
> + int nw = get_num_wrps();
> +
> + /* Copy across up to IMPDEF debug registers to our
> + * shadow copy in the vcpu structure. The hyp.S code
> + * will then set them up before we re-enter the guest.
> + */
> + memcpy(vcpu->arch.guest_debug_regs.dbg_bcr,
> + dbg->arch.dbg_bcr, sizeof(__u64)*nb);
> + memcpy(vcpu->arch.guest_debug_regs.dbg_bvr,
> + dbg->arch.dbg_bvr, sizeof(__u64)*nb);
> + memcpy(vcpu->arch.guest_debug_regs.dbg_wcr,
> + dbg->arch.dbg_wcr, sizeof(__u64)*nw);
> + memcpy(vcpu->arch.guest_debug_regs.dbg_wvr,
> + dbg->arch.dbg_wvr, sizeof(__u64)*nw);
> +
> + kvm_info("HW BP support requested\n");
> + for (i = 0; i < nb; i++) {
> + kvm_info("%d: dbg_bcr=0x%llx dbg_bvr=0x%llx\n",
> + i,
> + vcpu->arch.guest_debug_regs.dbg_bcr[i],
> + vcpu->arch.guest_debug_regs.dbg_bvr[i]);
> + }
> + for (i = 0; i < nw; i++) {
> + kvm_info("%d: dbg_wcr=0x%llx dbg_wvr=0x%llx\n",
> + i,
> + vcpu->arch.guest_debug_regs.dbg_wcr[i],
> + vcpu->arch.guest_debug_regs.dbg_wvr[i]);
> + }
I think we decided to drop the kvm_info's. Here's several more lines of
logging to drop too.
> + route_el2 = true;
> }
>
> /* If we are going to handle any debug exceptions we need to
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 52b484b..c450552 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
> }
> #endif
>
> +/* Determine number of BRP registers available. */
> +static inline int get_num_brps(void)
> +{
> + return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> +}
> +
> +/* Determine number of WRP registers available. */
> +static inline int get_num_wrps(void)
> +{
> + return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> +}
> +
> extern struct pmu perf_ops_bp;
>
> #endif /* __KERNEL__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 38b0f07..e386bf4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -103,8 +103,9 @@ struct kvm_vcpu_arch {
> /* Exception Information */
> struct kvm_vcpu_fault_info fault;
>
> - /* Debug state */
> + /* Guest debug state */
> u64 debug_flags;
> + struct kvm_guest_debug_arch guest_debug_regs;
>
> /* Pointer to host CPU context */
> kvm_cpu_context_t *host_cpu_context;
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 78e5ae1..c9ecfd3 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -122,6 +122,10 @@ int main(void)
> DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
> DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags));
> DEFINE(GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
> + DEFINE(GUEST_DEBUG_BCR, offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bcr));
> + DEFINE(GUEST_DEBUG_BVR, offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bvr));
> + DEFINE(GUEST_DEBUG_WCR, offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wcr));
> + DEFINE(GUEST_DEBUG_WVR, offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wvr));
> DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2));
> DEFINE(VCPU_MDCR_EL2, offsetof(struct kvm_vcpu, arch.mdcr_el2));
> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines));
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index df1cf15..45dcc6f 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
> static int core_num_brps;
> static int core_num_wrps;
>
> -/* Determine number of BRP registers available. */
> -static int get_num_brps(void)
> -{
> - return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> -}
> -
> -/* Determine number of WRP registers available. */
> -static int get_num_wrps(void)
> -{
> - return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> -}
> -
> int hw_breakpoint_slots(int type)
> {
> /*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 6def054..d024e47 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -110,6 +110,42 @@ static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
> return 0;
> }
>
> +/**
> + * kvm_handle_hw_bp - handle HW assisted break point
> + *
> + * @vcpu: the vcpu pointer
@run
> + *
> + */
> +static int kvm_handle_hw_bp(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> + WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
Another WARN_ON to consider dropping.
> +
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_BKPT;
> + run->debug.arch.address = *vcpu_pc(vcpu);
> + return 0;
> +}
> +
> +/**
> + * kvm_handle_watch - handle HW assisted watch point
> + *
> + * @vcpu: the vcpu pointer
@run
> + *
> + * These are basically the same as breakpoints (and indeed may use the
> + * breakpoint in a linked fashion). However they generate a specific
> + * exception so we trap it here for reporting to the guest.
> + *
> + */
> +static int kvm_handle_watch(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> + WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
> +
drop WARN_ON?
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_WTPT;
> + run->debug.arch.address = *vcpu_pc(vcpu);
> + return 0;
> +}
> +
> static exit_handle_fn arm_exit_handlers[] = {
> [ESR_EL2_EC_WFI] = kvm_handle_wfx,
> [ESR_EL2_EC_CP15_32] = kvm_handle_cp15_32,
> @@ -125,6 +161,8 @@ static exit_handle_fn arm_exit_handlers[] = {
> [ESR_EL2_EC_IABT] = kvm_handle_guest_abort,
> [ESR_EL2_EC_DABT] = kvm_handle_guest_abort,
> [ESR_EL2_EC_SOFTSTP] = kvm_handle_ss,
> + [ESR_EL2_EC_WATCHPT] = kvm_handle_watch,
> + [ESR_EL2_EC_BREAKPT] = kvm_handle_hw_bp,
> [ESR_EL2_EC_BKPT32] = kvm_handle_bkpt,
> [ESR_EL2_EC_BRK64] = kvm_handle_bkpt,
> };
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index b38ce3d..96f71ab 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -18,6 +18,7 @@
> #include <linux/linkage.h>
> #include <linux/kvm.h>
>
> +#include <uapi/asm/kvm.h>
> #include <asm/assembler.h>
> #include <asm/memory.h>
> #include <asm/asm-offsets.h>
> @@ -174,6 +175,7 @@
> ldr x3, [x0, #GUEST_DEBUG]
> tbz x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f // No guest debug
>
> + // Both Step and HW BP/WP ops need to modify spsr_el2 and mdscr_el1
Comment not quite right, since they both don't need to modify both.
Step modifies both. BP/WP modifies mdscr.
> // x0 - preserved as VCPU ptr
> // x1 - spsr
> // x2 - mdscr
> @@ -191,6 +193,11 @@
> eor x1, x1, #DBG_SPSR_SS
> eor x2, x2, #DBG_MDSCR_SS
> 1:
> + // If we are doing HW BP/WP - set MDSCR_EL1.KDE/MDE
> + tbz x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 3f
> + orr x2, x2, #DBG_MDSCR_KDE
> + orr x2, x2, #DBG_MDSCR_MDE
We don't need to make sure KDE/MDE are cleared when we're not doing
BP/WP, as we do with the SS bit?
> +3:
> msr spsr_el2, x1
> msr mdscr_el1, x2
> 2:
Not super pleasant to have the labels occur in 1,3,2 order, but
whatever.
> @@ -815,6 +822,33 @@ __restore_debug:
>
> ret
>
> +/* Setup debug state for debug of guest */
> +__setup_debug:
> + // x0: vcpu base address
> + // x3: ptr to guest registers passed to setup_debug_registers
> + // x5..x20/x26: trashed
Same comment on this header as for __restore_debug's new header in
a previous patch.
> +
> + mrs x26, id_aa64dfr0_el1
> + ubfx x24, x26, #12, #4 // Extract BRPs
> + ubfx x25, x26, #20, #4 // Extract WRPs
> + mov w26, #15
> + sub w24, w26, w24 // How many BPs to skip
> + sub w25, w26, w25 // How many WPs to skip
> +
> + mov x4, x24
> + add x3, x0, #GUEST_DEBUG_BCR
> + setup_debug_registers dbgbcr
Now I see why the name change of restore_debug to setup_debug_registers,
it fits better here in __setup_debug. Should we name is something that
fits both better?
> + add x3, x0, #GUEST_DEBUG_BVR
> + setup_debug_registers dbgbvr
> +
> + mov x4, x25
> + add x3, x0, #GUEST_DEBUG_WCR
> + setup_debug_registers dbgwcr
> + add x3, x0, #GUEST_DEBUG_WVR
> + setup_debug_registers dbgwvr
> +
> + ret
> +
> __save_fpsimd:
> save_fpsimd
> ret
> @@ -861,6 +895,13 @@ ENTRY(__kvm_vcpu_run)
> bl __restore_sysregs
> bl __restore_fpsimd
>
> + // Now is the time to set-up the debug registers if we
> + // are debugging the guest
^^^ whitespace
> + ldr x3, [x0, #GUEST_DEBUG]
> + tbz x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 2f
> + bl __setup_debug
> + b 1f
> +2:
> skip_debug_state x3, 1f
> bl __restore_debug
> 1:
> @@ -881,6 +922,11 @@ __kvm_vcpu_return:
> bl __save_fpsimd
> bl __save_sysregs
>
> + // If we are debugging the guest don't save debug registers
> + // otherwise we'll be trashing are only good copy we have.
^^ the
> + ldr x3, [x0, #GUEST_DEBUG]
> + tbnz x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 1f
> +
> skip_debug_state x3, 1f
> bl __save_debug
I feel like there should be a more elegant way to merge the selection of
__setup_debug vs. __restore_debug and skip_debug_state.
> 1:
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 70a7816..0de6caa 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -64,6 +64,12 @@ int kvm_arch_dev_ioctl_check_extension(long ext)
> case KVM_CAP_ARM_EL1_32BIT:
> r = cpu_has_32bit_el1();
> break;
> + case KVM_CAP_GUEST_DEBUG_HW_BPS:
> + r = get_num_brps();
> + break;
> + case KVM_CAP_GUEST_DEBUG_HW_WPS:
> + r = get_num_wrps();
^ extra space here
> + break;
> default:
> r = 0;
> }
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 347e5b0..49a5f97 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -759,6 +759,8 @@ struct kvm_ppc_smmu_info {
> #define KVM_CAP_PPC_FIXUP_HCALL 103
> #define KVM_CAP_PPC_ENABLE_HCALL 104
> #define KVM_CAP_CHECK_EXTENSION_VM 105
> +#define KVM_CAP_GUEST_DEBUG_HW_BPS 106
> +#define KVM_CAP_GUEST_DEBUG_HW_WPS 107
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> --
> 2.1.3
>
> _______________________________________________
> kvmarm mailing list
> kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply
* Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step
From: Andrew Jones @ 2014-11-26 16:40 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A, marc.zyngier-5wv7dgnIgG8,
peter.maydell-QSEj5FYQhm4dnm+yROfE0A, agraf-l3A5Bk7waGM,
Lorenzo Pieralisi, Russell King, Gleb Natapov,
jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ, Will Deacon, open list,
open list:ABI/API, dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
Catalin Marinas, r65777-KZfg59tc24xl57MIdRCFDg,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, bp-l3A5Bk7waGM
In-Reply-To: <1416931805-23223-6-git-send-email-alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
> This adds support for single-stepping the guest. As userspace can and
> will manipulate guest registers before restarting any tweaking of the
> registers has to occur just before control is passed back to the guest.
> Furthermore while guest debugging is in effect we need to squash the
> ability of the guest to single-step itself as we have no easy way of
> re-entering the guest after the exception has been delivered to the
> hypervisor.
>
> Signed-off-by: Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 48d26bb..a76daae 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -38,6 +38,7 @@
> #include <asm/tlbflush.h>
> #include <asm/cacheflush.h>
> #include <asm/virt.h>
> +#include <asm/debug-monitors.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_asm.h>
> #include <asm/kvm_mmu.h>
> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> kvm_arm_set_running_vcpu(NULL);
> }
>
> +/**
> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
> + * @kvm: pointer to the KVM struct
> + * @kvm_guest_debug: the ioctl data buffer
> + *
> + * This sets up the VM for guest debugging. Care has to be taken when
> + * manipulating guest registers as these will be set/cleared by the
> + * hyper-visor controller, typically before each kvm_run event. As a
> + * result modification of the guest registers needs to take place
> + * after they have been restored in the hyp.S trampoline code.
> + */
> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> struct kvm_guest_debug *dbg)
> {
> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>
> /* Single Step */
> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> - kvm_info("SS requested, not yet implemented\n");
> - return -EINVAL;
> + kvm_info("SS requested\n");
> + route_el2 = true;
> }
>
> /* Software Break Points */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 8da1043..78e5ae1 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -121,6 +121,7 @@ int main(void)
> DEFINE(VCPU_FAR_EL2, offsetof(struct kvm_vcpu, arch.fault.far_el2));
> DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
> DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags));
> + DEFINE(GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
> DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2));
> DEFINE(VCPU_MDCR_EL2, offsetof(struct kvm_vcpu, arch.mdcr_el2));
> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines));
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 28dc92b..6def054 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
> return 0;
> }
>
> +/**
> + * kvm_handle_ss - handle single step exceptions
> + *
> + * @vcpu: the vcpu pointer
same @run comment as other handler header in previous patch
> + *
> + * See: ARM ARM D2.12 for the details. While the host is routing debug
> + * exceptions to it's handlers we have to suppress the ability of the
> + * guest to trigger exceptions.
> + */
> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> + WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
I'm not sure about this WARN_ON. Is there some scenario you were
thinking of when you put it here? Is there some scenario where this
could trigger so frequently we kill the log buffer?
> +
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.exit_type = KVM_DEBUG_EXIT_SINGLE_STEP;
> + run->debug.arch.address = *vcpu_pc(vcpu);
> + return 0;
> +}
> +
> static exit_handle_fn arm_exit_handlers[] = {
> [ESR_EL2_EC_WFI] = kvm_handle_wfx,
> [ESR_EL2_EC_CP15_32] = kvm_handle_cp15_32,
> @@ -105,6 +124,7 @@ static exit_handle_fn arm_exit_handlers[] = {
> [ESR_EL2_EC_SYS64] = kvm_handle_sys_reg,
> [ESR_EL2_EC_IABT] = kvm_handle_guest_abort,
> [ESR_EL2_EC_DABT] = kvm_handle_guest_abort,
> + [ESR_EL2_EC_SOFTSTP] = kvm_handle_ss,
> [ESR_EL2_EC_BKPT32] = kvm_handle_bkpt,
> [ESR_EL2_EC_BRK64] = kvm_handle_bkpt,
> };
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 3c733ea..c0bc218 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -16,6 +16,7 @@
> */
>
> #include <linux/linkage.h>
> +#include <linux/kvm.h>
>
> #include <asm/assembler.h>
> #include <asm/memory.h>
> @@ -168,6 +169,31 @@
> // x19-x29, lr, sp*, elr*, spsr*
> restore_common_regs
>
> + // After restoring the guest registers but before we return to the guest
> + // we may want to make some final tweaks to support guest debugging.
> + ldr x3, [x0, #GUEST_DEBUG]
> + tbz x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f // No guest debug
> +
> + // x0 - preserved as VCPU ptr
> + // x1 - spsr
> + // x2 - mdscr
> + mrs x1, spsr_el2
> + mrs x2, mdscr_el1
> +
> + // See ARM ARM D2.12.3 The software step state machine
> + // If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
> + orr x1, x1, #DBG_SPSR_SS
> + orr x2, x2, #DBG_MDSCR_SS
> + tbnz x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
> + // If we are not doing Single Step we want to prevent the guest doing so
> + // as otherwise we will have to deal with the re-routed exceptions as we
> + // are doing other guest debug related things
> + eor x1, x1, #DBG_SPSR_SS
> + eor x2, x2, #DBG_MDSCR_SS
> +1:
> + msr spsr_el2, x1
> + msr mdscr_el1, x2
> +2:
> // Last bits of the 64bit state
> pop x2, x3
> pop x0, x1
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 523f476..347e5b0 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -7,6 +7,8 @@
> * Note: you must update KVM_API_VERSION if you change this interface.
> */
>
> +#ifndef __ASSEMBLY__
> +
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>
> @@ -515,11 +517,6 @@ struct kvm_s390_irq {
> } u;
> };
>
> -/* for KVM_SET_GUEST_DEBUG */
> -
> -#define KVM_GUESTDBG_ENABLE 0x00000001
> -#define KVM_GUESTDBG_SINGLESTEP 0x00000002
> -
> struct kvm_guest_debug {
> __u32 control;
> __u32 pad;
> @@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
> __u16 padding[3];
> };
>
> +#endif /* __ASSEMBLY__ */
> +
> +/* for KVM_SET_GUEST_DEBUG */
> +
> +#define KVM_GUESTDBG_ENABLE_SHIFT 0
> +#define KVM_GUESTDBG_ENABLE (1 << KVM_GUESTDBG_ENABLE_SHIFT)
> +#define KVM_GUESTDBG_SINGLESTEP_SHIFT 1
> +#define KVM_GUESTDBG_SINGLESTEP (1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)
EALIGN: we can tab these defines up better
> +
> +
> +
> #endif /* __LINUX_KVM_H */
> --
> 2.1.3
>
> _______________________________________________
> kvmarm mailing list
> kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply
* Re: [PATCH v4 13/42] virtio_blk: v1.0 support
From: Michael S. Tsirkin @ 2014-11-26 15:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: Thomas Huth, rusty, linux-api, linux-kernel, virtualization,
pbonzini, David Miller
In-Reply-To: <20141126164045.0e9bfadd@thinkpad-w530>
On Wed, Nov 26, 2014 at 04:40:45PM +0100, David Hildenbrand wrote:
> > On Tue, Nov 25, 2014 at 06:55:16PM +0100, Cornelia Huck wrote:
> > > On Tue, 25 Nov 2014 18:42:18 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > Based on patch by Cornelia Huck.
> > > >
> > > > Note: for consistency, and to avoid sparse errors,
> > > > convert all fields, even those no longer in use
> > > > for virtio v1.0.
> > > >
> > > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > > > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > include/uapi/linux/virtio_blk.h | 15 ++++-----
> > > > drivers/block/virtio_blk.c | 70 ++++++++++++++++++++++++-----------------
> > > > 2 files changed, 49 insertions(+), 36 deletions(-)
> > > >
> > >
> > > Is this sufficiently unchanged so that the original r-bs still hold?
> >
> > You are right, I'm afraid I'll have to drop these.
> > Thomas, David, care to comment?
>
> I've got this on my list, will try to find some spare time tomorrow!
>
> > Even better - have the cycles to review the latest version?
>
> Yes, will try.
>
> >
Thanks!
If/when you do, pls make sure to pick the latest version.
You can always get it from vhost-net branch in my tree.
I will likely post another version tomorrow: Cornelia
found a bug in virtio-scsi.
--
MST
^ permalink raw reply
* Re: [PATCH v4 13/42] virtio_blk: v1.0 support
From: David Hildenbrand @ 2014-11-26 15:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Thomas Huth, rusty, linux-api, linux-kernel, virtualization,
pbonzini, David Miller
In-Reply-To: <20141125214342.GE22343@redhat.com>
> On Tue, Nov 25, 2014 at 06:55:16PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:42:18 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > Based on patch by Cornelia Huck.
> > >
> > > Note: for consistency, and to avoid sparse errors,
> > > convert all fields, even those no longer in use
> > > for virtio v1.0.
> > >
> > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > include/uapi/linux/virtio_blk.h | 15 ++++-----
> > > drivers/block/virtio_blk.c | 70 ++++++++++++++++++++++++-----------------
> > > 2 files changed, 49 insertions(+), 36 deletions(-)
> > >
> >
> > Is this sufficiently unchanged so that the original r-bs still hold?
>
> You are right, I'm afraid I'll have to drop these.
> Thomas, David, care to comment?
I've got this on my list, will try to find some spare time tomorrow!
> Even better - have the cycles to review the latest version?
Yes, will try.
>
^ permalink raw reply
* Re: kdbus: add documentation
From: Andy Lutomirski @ 2014-11-26 15:39 UTC (permalink / raw)
To: David Herrmann
Cc: Greg Kroah-Hartman, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Tom Gundersen, Jiri Kosina, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Daniel Mack,
Djalal Harouni
In-Reply-To: <CALCETrV1vChd9m_AtFjPfddqvPK3z3cjROozWJ8HQHSGm5KWJQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Nov 26, 2014 at 7:30 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> Then find a clean way that's gated on having the right /proc access,
> which is not guaranteed to exist on all of your eventual users'
> systems, and, if that access doesn't exist because the admin or
> sandbox designer has sensibly revoked it, then kdbus shouldn't
> override them.
One idea: add a sysctl that defaults to off that enables these
metadata items, and keep it disabled on production systems. Then you
get your debugging and everyone else gets unsurprising behavior.
--Andy
>
> --Andy
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: kdbus: add documentation
From: Andy Lutomirski @ 2014-11-26 15:30 UTC (permalink / raw)
To: David Herrmann
Cc: Greg Kroah-Hartman, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Tom Gundersen, Jiri Kosina, Linux API,
linux-kernel@vger.kernel.org, Daniel Mack, Djalal Harouni
In-Reply-To: <CANq1E4TOe5SNmAvnjs1LCx9EGrGWxqX8qPUy26n8RzGo2kvThg@mail.gmail.com>
On Wed, Nov 26, 2014 at 3:55 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Nov 24, 2014 at 9:57 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Mon, Nov 24, 2014 at 12:16 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> [snip]
>>>>> +6.5 Getting information about a connection's bus creator
>>>>> +--------------------------------------------------------
>>>>> +
>>>>> +The KDBUS_CMD_BUS_CREATOR_INFO ioctl takes the same struct as
>>>>> +KDBUS_CMD_CONN_INFO but is used to retrieve information about the creator of
>>>>> +the bus the connection is attached to. The metadata returned by this call is
>>>>> +collected during the creation of the bus and is never altered afterwards, so
>>>>> +it provides pristine information on the task that created the bus, at the
>>>>> +moment when it did so.
>>>>
>>>> What's this for? I understand the need for the creator of busses to
>>>> be authenticated, but doing it like this mean that anyone who will
>>>> *fail* authentication can DoS the authentic creator.
>>>
>>> This returns information on a bus owner, to determine whether a
>>> connection is connected to a system, user or session bus. Note that
>>> the bus-creator itself is not a valid peer on the bus, so you cannot
>>> send messages to them. Which kind of DoS do you have in mind?
>>
>> I assume that the logic is something like:
>>
>> connect to bus
>> request bus metadata
>> if (bus metadata matches expectations) {
>> great, trust the bus!
>> } else {
>> oh crap!
>> }
>
> Uh, no, this is really not the logic that should be assumed. It's more
> for code where you want to simply pass a bus fd, and the code knows
> nothing about it. Now, the code can derive some information from the
> bus fd, like for example who owns it. Then, depending on some of the
> creds returned it can determine whether to read configuration file set
> A or B and so on. This is particularly useful for all kinds of
> unprivileged bus services that end up running on any kind of bus and
> need to be able to figure out what they are actually operating on.
The logic you've described is more or less the same thing that I
described with a process transition in. It's:
connect to bus
send kdbus fd to another process, which does the rest:
request bus metadata
if (bus metadata matches expectations) {
great, trust the bus!
} else {
oh crap! (or malfunction or whatever)
}
ISTM you should have an API to get the *name* of the bus and check that.
Except that, if the service you pass that fd to is privileged, then
you're completely screwed, because none of this checks that the
*domain* is correct.
[snip]
>
>>>>> +
>>>>> +Also, if the connection allowed for file descriptor to be passed
>>>>> +(KDBUS_HELLO_ACCEPT_FD), and if the message contained any, they will be
>>>>> +installed into the receiving process after the KDBUS_CMD_MSG_RECV ioctl
>>>>> +returns. The receiving task is obliged to close all of them appropriately.
>>>>
>>>> This makes it sound like fds are installed at receive time. What
>>>> prevents resource exhaustion due to having excessive numbers of fds in
>>>> transit (that are presumably not accounted to anyone)?
>>>
>>> We have a per-user message accounting for undelivered messages, as
>>> well as a maximum number of pending messages per connection on the
>>> receiving end. These limits are accounted on a "user<->user" basis, so
>>> the limit of a user A will not affect two other users (B and C)
>>> talking.
>>
>> But you can shove tons of fds in a message, and you can have lots of
>> messages, and some of the fds can be fds of unix sockets that have fds
>> queued up in them, and one of those fds could be the fd to the kdbus
>> connection that sent the fd...
>
> You cannot send kdbus-fds or unix-fds over kdbus, right now. We have
> people working on the AF_UNIX gc to make it more generic and include
> external types. Until then, we simply prevent recursive fd passing.
OK, fair enough.
>
>> This is not advice as to what to do about it, but I think that it will
>> be a problem at some point.
>>
>>
>>>>> +11. Policy
>>>>> +===============================================================================
>>>>> +
>>>>> +A policy databases restrict the possibilities of connections to own, see and
>>>>> +talk to well-known names. It can be associated with a bus (through a policy
>>>>> +holder connection) or a custom endpoint.
>>>>
>>>> ISTM metadata items on bus names should be replaced with policy that
>>>> applies to the domain as a whole and governs bus creation.
>>>
>>> No, well-known names are bound to buses, so a bus is really the right
>>> place to hold policy about which process is allowed to claim them.
>>> Every user is allowed to create a bus of its own, there's no policy
>>> for that, and there shouldn't be.
>>>
>>> It has nothing to do with metadata items.
>>
>> But it does -- the creator of the bus binds metadata to that bus at
>> creation time.
>>
>> I think that a better solution would be to have a global policy that
>> says, for example, "to create the bus called 'system', the creator
>> must have selinux label xyz" or "to create a user bus called
>> uid-1000-privileged-ui-bus the creator must have some cgroup" or
>> whatever.
>>
>> Although maybe a better solution would leave this in the kernel but
>> allow any cgroup to create a bus with a same that indicates the
>> creating cgroup. Then I could have my desktop shell create a
>> "/cgroup/path/to/desktop" for per-user privileged things.
>
> We enforce the UID as first entity of the bus name. Again, this is our
> default policy because we rely on user-based access control. If we
> want more fine-grained access-control, we can introduce other policies
> at any time. For instance, we could enforce "cg-<cgroup>-<busname>"
> later on, where the kernel requires the caller to prefix the bus with
> "cg-<cgroup>-", where <cgroup> is the cgroup-path encoded in some way.
>
> We provide one policy as default, and we have a use-case for it.
> Further policies are always welcome as extensions later on. I don't
> see why we should provide all those right from the beginning without
> any users right now.
OK
>
>>>
>>>>> +A set of policy rules is described by a name and multiple access rules, defined
>>>>> +by the following struct.
>>>>> +
>>>>> +struct kdbus_policy_access {
>>>>> + __u64 type; /* USER, GROUP, WORLD */
>>>>> + One of the following.
>>>>> +
>>>>> + KDBUS_POLICY_ACCESS_USER
>>>>> + Grant access to a user with the uid stored in the 'id' field.
>>>>> +
>>>>> + KDBUS_POLICY_ACCESS_GROUP
>>>>> + Grant access to a user with the gid stored in the 'id' field.
>>>>> +
>>>>> + KDBUS_POLICY_ACCESS_WORLD
>>>>> + Grant access to everyone. The 'id' field is ignored.
>>>>> +
>>>>> + __u64 access; /* OWN, TALK, SEE */
>>>>> + The access to grant.
>>>>> +
>>>>> + KDBUS_POLICY_SEE
>>>>> + Allow the name to be seen.
>>>>> +
>>>>> + KDBUS_POLICY_TALK
>>>>> + Allow the name to be talked to.
>>>>> +
>>>>> + KDBUS_POLICY_OWN
>>>>> + Allow the name to be owned.
>>>>> +
>>>>> + __u64 id;
>>>>> + For KDBUS_POLICY_ACCESS_USER, stores the uid.
>>>>> + For KDBUS_POLICY_ACCESS_GROUP, stores the gid.
>>>>> +};
>>>>
>>>>
>>>> What happens if there are multiple matches?
>>>
>>> We only have _granting_ policy entries. We search through the
>>> policy-db until we find an entry that grants access. We do _not_ stop
>>> on the first item that matches.
>>
>> Yay! Can you document that more clearly?
>
> Sure!
>
>>>
>>>>
>>>>> +
>>>>> +11.4 TALK access and multiple well-known names per connection
>>>>> +-------------------------------------------------------------
>>>>> +
>>>>> +Note that TALK access is checked against all names of a connection.
>>>>> +For example, if a connection owns both 'org.foo.bar' and 'org.blah.baz', and
>>>>> +the policy database allows 'org.blah.baz' to be talked to by WORLD, then this
>>>>> +permission is also granted to 'org.foo.bar'. That might sound illogical, but
>>>>> +after all, we allow messages to be directed to either the name or a well-known
>>>>> +name, and policy is applied to the connection, not the name. In other words,
>>>>> +the effective TALK policy for a connection is the most permissive of all names
>>>>> +the connection owns.
>>>>
>>>> This does seem illogical. Does the recipient at least know which
>>>> well-known name was addressed?
>>>
>>> If the sender addressed it to a well-known name, yes. If the sender
>>> addressed the message to a unique-ID, there will be no such name, of
>>> course. Still, the policy applies to such transactions either way
>>> (standard D-Bus behavior).
>>>
>>> Note however that dbus1 will not pass along the destination well-known
>>> name, hence most userspace libraries will ignore this information too,
>>> even if they run on kdbus which might pass this information around.
>>> The right way for services that carry multiple service names to
>>> discern which actual service is being talked to is having separate
>>> object paths for the different functionality to hide between the
>>> services.
>>
>> It seems unfortunate to keep around this really weird behavior for the
>> benefit of legacy applications. Could there perhaps be a flag that a
>> connection can set to indicate that it understands per-destination
>> access control and therefore wants stricter policy enforcement?
>
> We actually think that it's a good idea not to use the destination
> name for doing different things, to make things more transparent. For
> example, we have tools that can explore the bus, introspect things
> (like d-bus), and they should show the same objects regardless by
> which name you access a service. It's more transparent then, and
> really just reduces names to some labels that make addressing things
> easier, but that are actually completely unnecessary for actual method
> invocations.
>
> This behaviour is also relied on by a number of current bindings. For
> example GLib's implementation usually caches the unique name of a
> peer, and uses that for talking to remote objects (rather than always
> using the well-known name), in order to get an error back when a name
> becomes unavailable (maybe because a service died) or is moved to a
> different peer. If daemons would always take the destination name into
> account this kind of logic could never work.
>
> It does take some time to get used to the fact that names are
> exclusively used for message routing and policies, not as
> target-entity of actual method calls. But the current dbus1 behaviour
> makes a ton of sense, and is really something we want to keep. It
> improves how clients can do life-cycle tracking of remote objects.
>
> Note that D-Bus modeled unique-names and well-known-names after IP
> addresses and DNS names. It's a very similar model, and, like DNS
> names, well-known names have no effect on the routing of messages.
DNS names also have no effect on your ability to send to an IP.
In any event, I think you can keep doing everything that you're
currently doing if you check policy on the actual sent-to name. Users
will just apply the *same* policy to all of their names (or just use
one name), and everything will work. And, for bonus points, the
security rules will make sense.
>
>>>
>>>>> +11.5 Implicit policies
>>>>> +----------------------
>>>>> +
>>>>> +Depending on the type of the endpoint, a set of implicit rules might be
>>>>> +enforced. On default endpoints, the following set is enforced:
>>>>> +
>>>>
>>>> How do these rules interact with installed policy?
>>>
>>> As said before, all policy entries _grant_ access. We look through all
>>> entries until we find one that grants access.
>>>
>>>>> + * Privileged connections always override any installed policy. Those
>>>>> + connections could easily install their own policies, so there is no
>>>>> + reason to enforce installed policies.
>>>>> + * Connections can always talk to connections of the same user. This
>>>>> + includes broadcast messages.
>>>>
>>>> Why?
>>>
>>> All limits on buses are enforced on a user<->user basis. We don't want
>>> to provide policies that are more fine-grained than our accounting.
>>
>> This seems completely at odds with all the fine-grained metadata
>> stuff. Also, anything that relies on this may get very confused when
>> the LSM hooks go in, because I'm reasonably sure that the intent is
>> for them to *not* follow this principle.
>
> User-based accounting has always been the default, right? We are open
> to extend the API to support any other accounting scheme (LSM,
> cgroup-based, ...). But like bus-name-policies, I think it's fine to
> keep this as future extension. If you think the current design
> precludes LSM-based accounting, lemme know and we can fix it. But we
> have talked to LSM people before (and there have been patches on
> LKML), and they seemed fine with it.
OK, as long as restricted endpoints don't work like this.
>
>>>
>>>> If anyone ever strengthens the concept of identity to include
>>>> things other than users (hmm -- there are already groups), this could
>>>> be very limiting.
>>>
>>> If user-based accounting is not suitable, you can create custom
>>> endpoints. Future extensions to that are always welcome. So far, the
>>> default user-based accounting was enough. And I think it's suitable as
>>> default.
>>>
>>>>> + * Connections that own names might send broadcast messages to other
>>>>> + connections that belong to a different user, but only if that
>>>>> + destination connection does not own any name.
>>>>> +
>>
>> (Also, what does "might" mean here?)
>>
>>>>
>>>> This is weird. It is also differently illogical than the "illogical"
>>>> thing above.
>>>
>>> Actually it follows the same model described above. If two connections
>>> are running under the same user then broadcasts are allowed, but if
>>> they are running under different users *and* if the destination owns a
>>> well-known name, then broadcasts are subject to TALK policy checks
>>> since that destination may own a restricted well-known name that is
>>> not interested in broadcasts. So this implicit policy is just
>>> fast-path for the common case where the target is subscribed to a
>>> broadcast and does not own any name.
>>
>> Huh?
>>
>> Say I have two users, "Sender" and "Receiver", each with a single
>> connection. If Receiver owns no well-known names, then Sender can
>> send to it. If Receiver owns one well-known name, then Sender needs
>> to pass a TALK check on that name. If Reciever owns two well-known
>> names, then Sender only needs to pass a TALK check on one of them.
>>
>> Am I understanding this right? If I am, then I think this is in the
>> category of baroque and inconsistent security rules which everyone
>> will screw up and therefore introduce security vulnerabilities.
>>
>> Can you really not enforce the much simpler rule that, to send to a
>> name, you must have permission to send to *that* name? If legacy
>> dbus1 receivers register two names and don't validate everything
>> correctly, then only the legacy receivers have problems.
>
> Sorry, I got confused here. That implicit policy is now dropped.
>
Sounds good.
>>>
>>>>> +
>>>>> +13. Metadata
>>>>> +===============================================================================
>>> [snip]
>>>>> +13.1 Known item types
>>>>> +---------------------
>>>>> +
>>>>> +The following attach flags are currently supported.
>>>>> +
>>>>> + KDBUS_ATTACH_TIMESTAMP
>>>>> + Attaches an item of type KDBUS_ITEM_TIMESTAMP which contains both the
>>>>> + monotonic and the realtime timestamp, taken when the message was
>>>>> + processed on the kernel side.
>>>>> +
>>>>> + KDBUS_ATTACH_CREDS
>>>>> + Attaches an item of type KDBUS_ITEM_CREDS, containing credentials as
>>>>> + described in kdbus_creds: the uid, gid, pid, tid and starttime of the task.
>>>>> +
>>>>
>>>> As mentioned last time, please remove or justify starttime.
>>>
>>> starttime allows detecting PID overflows. Exposing the process
>>> starttime is useful to detect when a PID is getting reused.
>>> Unfortunately, we don't have 64bit pids, so we need the pid+time
>>> combination to avoid ambiguity.
>>
>> NAK, I think.
>>
>> I agree that PID overflow is a real issue and should be addressed
>> somehow. But please address it for real instead of adding Yet Another
>> Hack (tm). In the mean time, leave that hack out, please.
>>
>> I would *love* to see PIDs have extra high bits at the end, done in a
>> way that supports CRIU and that guarantees no reuse unless something
>> privileged intentionally mis-programs it. But starttime isn't that
>> mechanism.
>
> The starttime logic sufficiently fixes the issue. If one great day in
> the future somebody invents some completely new concept for making
> this problem go away, we can look into that, but even then the field
> is still valuable for informational purposes. I mean, the kernel
> tracks this and exposes this in /proc for a reason...
>
> In the meantime, we don't have any other way of solving this problem,
> so we'll leave this in.
>
But this set of patches doesn't "leave this in". This set of patches
*adds it*. Designing a poor new API is not excused by the fact that
no one has developed the better one yet.
>>>
>>>>> + KDBUS_ATTACH_AUXGROUPS
>>>>> + Attaches an item of type KDBUS_ITEM_AUXGROUPS, containing a dynamic
>>>>> + number of auxiliary groups the sending task was a member of.
>>>>> +
>>>>> + KDBUS_ATTACH_NAMES
>>>>> + Attaches items of type KDBUS_ITEM_OWNED_NAME, one for each name the sending
>>>>> + connection currently owns. The name and flags are stored in kdbus_item.name
>>>>> + for each of them.
>>>>> +
>>>>
>>>> That's interesting. What's it for?
>>>
>>> It a valuable piece of information for receivers to know which bus
>>> names a sender has claimed. For instance, we need this information for
>>> the D-Bus proxy service, because we have to apply D-Bus1 policy in
>>> that case, and we need to get a list of owned names in a race-free
>>> manner to check the policy against.
>>
>> But if you change the rule to the sensible one where you need
>> permission to TALK to the name that you're talking to, this goes away,
>> right?
>
> This does not work if a message is directed at a unique-name, as
> explained above (or, think broadcasts).
Is the issue that, if I send a proxied request to unique name foo, and
unique name foo is help by someone who's claimed /a/b/c, then the
proxy service needs to check for permission to talk to /a/b/c?
To me, this still feels like this is only an issue because of this
weird concept of how names and policies interact. I really think that
you should put some extra effort into making the policy matches
self-contained in the sense of only matching on addressing information
that's actually in the message.
For example, HELLO could indicate one of "anyone can talk to me" or
"no one can talk to me without using a well-known name", then the
problem is solved. Protocols that switch to using the unique name
(e.g. glib) can send *both* the unique and well-known name and the
access control will still work.
>
>>>
>>>>> + KDBUS_ATTACH_TID_COMM
>>>>> + Attaches an items of type KDBUS_ITEM_TID_COMM, transporting the sending
>>>>> + task's 'comm', for the tid. The string is stored in kdbus_item.str.
>>>>> +
>>>>> + KDBUS_ATTACH_PID_COMM
>>>>> + Attaches an items of type KDBUS_ITEM_PID_COMM, transporting the sending
>>>>> + task's 'comm', for the pid. The string is stored in kdbus_item.str.
>>>>> +
>>>>> + KDBUS_ATTACH_EXE
>>>>> + Attaches an item of type KDBUS_ITEM_EXE, containing the path to the
>>>>> + executable of the sending task, stored in kdbus_item.str.
>>>>> +
>>>>> + KDBUS_ATTACH_CMDLINE
>>>>> + Attaches an item of type KDBUS_ITEM_CMDLINE, containing the command line
>>>>> + arguments of the sending task, as an array of strings, stored in
>>>>> + kdbus_item.str.
>>>>
>>>> Please remove these four items. They are genuinely useless. Anything
>>>> that uses them for anything is either buggy or should have asked the
>>>> sender to put the value in the payload (and immediately wondered why
>>>> it was doing that).
>>>
>>> We use them for logging, debugging and monitoring. With our wireshark
>>> extension it's pretty useful to know the comm-name of a process when
>>> monitoring a bus. As we explained last time, this is not about
>>> security. We're aware that a process can modify them. We use them only
>>> as additional meta-data for logging and debugging.
>>
>> Use the PID. Really. Your wireshark extention can look this crap up
>> in /proc and, if it fails due to a race, big frickin' deal.
>
> I see no reason for leaving it up to the client to do this extra work
> if it can as well be attached by the kernel, really.
>
> We use the PID on dbus1 systems for cases like this. But it's actually
> too racy to be useful. For example, in systemd we ship a tiny binary
> that is used as cgroup agent, which just pushes a message about the
> fact it just got called for a cgroup onto the bus and then exits.
> Since it only runs for a very very short time, any peer which then
> tries to read the metadata off it is pretty likely to fail. And there
> are quite a number of processes like that, that just do one thing and
> die, especially in the early boot process. For none of them we
> currently can generate useful log metadata, because all we have is a
> PID that we have barely any useful information about.
> This is a real race we get lots of bug-reports for. With kdbus we want
> to fix this, by optionally attaching this useful data to the message,
> so that the receiver can get the information when it wants to.
>
I really don't think that this kind of mostly indiscriminate
broadcasting of almost-invariably unnecessary and potentially
dangerous information is justified by the limited logging and
debugging benefit. I think you should find a better design that
solves your problem.
[snip]
>> Transparency is a terrible thing here.
>>
>> How many users put passwords into things on the command line? Yes,
>> it's a bad idea (for reasons that are entirely stupid), but now those
>> passwords get *logged*.
>>
>> If this is in the kernel, and someone complains that sensitive data is
>> showing up on ten different logs on their system, they'll *correctly*
>> blame the kernel. If you at least use the PID and restrict it to the
>> logging code, then at least the bug report will go to the logging
>> daemon, which will be *correctly* accused of doing something daft, and
>> it can be fixed.
>
> Hmm? Not following here. This information is visible via /proc too. If
> you hide it from /proc via the hide_pid logic, then it is also gone
> from the kdbus meta-data. Also, note again that clients that don't
> want this information to be passed to services can declare that now
> with their sender creds mask, introduced with v2.
>
We're talking about users, not senders, and users aren't going to
expect that they're sending their command line arond the bus. And
there could also receivers who are sandboxed and can't access /proc.
My point is that this information is probably okay to expose to
privileged logging daemons, and *maybe* to journald, but it's not okay
to pass anywhere else.
>>>
>>>>> +
>>>>> + KDBUS_ATTACH_CGROUP
>>>>> + Attaches an item of type KDBUS_ITEM_CGROUP with the task's cgroup path.
>>>>> +
>>>>> + KDBUS_ATTACH_CAPS
>>>>> + Attaches an item of type KDBUS_ITEM_CAPS, carrying sets of capabilities
>>>>> + that should be accessed via kdbus_item.caps.caps. Also, userspace should
>>>>> + be written in a way that it takes kdbus_item.caps.last_cap into account,
>>>>> + and derive the number of sets and rows from the item size and the reported
>>>>> + number of valid capability bits.
>>>>> +
>>>>
>>>> Please remove this, too, or justify its use.
>>>
>>> cgroup information tells us which service is doing a bus request. This
>>> is useful for a variety of things. For example, the bus activation
>>> logging item above benefits from it. In general, if some message shall
>>> be logged about any client it is useful to know its service name.
>>>
>>> Capabilities are useful to authenticate specific method calls. For
>>> example, when a client asks systemd to reboot, without this concept we
>>> can only check for UID==0 to decide whether to allow this. Breaking
>>> this down to capabilities in a race-free way has the benefit of
>>> allowing systemd to bind this to CAP_SYS_BOOT instead. There is no
>>> reason to deny a process with CAP_SYS_BOOT to reboot via bus-APIs, as
>>> they could just enforce it via syscalls, anyway.
>>
>> With all due respect, BS.
>>
>> I admit that there is probably no reason to deny systemd-based reboot
>> to a CAP_SYS_BOOT-capable process, but there is absolutely no reason
>> to give processes that are supposed to reboot using systemd the
>> CAP_SYS_BOOT capability.
>
> No, and this is not how this works. Note that for unpriviliged clients
> systemd checks PolicyKit in order to identify whether to allow certain
> priviliged operations. However, PolicyKit requires bus chatte, is slow
> and quite complex, hence the code shortcuts things if it knows that
> the client is priviliged anyway, and doesn't even bother with
> PolicyKit at all. This is where the caps come into play, since this
> shortcutting really should *NOT* be done for every single client, but
> only for those with the rights to do the operation anyway.
>
> Hence, this is really not about overloading capabilities with new
> meanings. Instead it is about shortcutting policykit for priviliged
> clients. And this shortcutting should be as restricted as possible.
So this mechanism is there to speed up reboot and similar things?
>
>> In any event, I suspect you'll have a hard time justifying this for
>> anything other than CAP_SYS_BOOT. Just because CAP_SYS_ADMIN users
>> can probably do whatever they want doesn't mean that systemd should
>> make that a built-in policy.
>
> Well, the other option for these APIs is to use the euid, which is
> hardly any better.
It absolutely is better. You can, in software, change the rules, and
euid works intelligently in namespaces. Aside from being a bad idea,
caps are just *broken* without knowledge of the userns hierarchy *and*
userns ownership information.
ns_capable is the in-kernel interface, but the return value of
ns_capable can't be calculated from just the cap mask.
Also, have you ever tried to usefully assign caps to euid != 0 users?
It's essentially entirely broken, especially if you try to use execve.
Don't force the use of the crappy fscaps interface to allow non-root
to reboot cleanly without invoking policykit -- you won't be doing
anyone any favors. Just set your userspace policy the way you want
it.
I've tried to fix this, I've written patches, I've designed
alternative and IMO far superior cap transition rules. Guess what?
They're not in the kernel because no one wants to touch caps. But
that doesn't mean that the existing cap system is anything other than
a giant pile of cr*p that happens to sort of work. Don't make user
code have to deal with the mess more than it already has to.
>
> systemd-timedated shortcuts policykit if the client has CAP_SYS_TIME
> and tries to change the system clock. Similar, if a process asks
> logind to kill a session, we bypass pk if the client has CAP_KILL.
>
>> Also, wtf is the bounding set and such for? At the very least this
>> should only be the effective set.
>
> Yes, the code that makes use of this for shortcutting pk uses the
> effective set only. The other ones we allow sending across for
> enhancing logging of security operations.
>
> In general: all creds we collect at the very least are incredibly
> useful for generating log records in a race-free fashion. As pointed
> out above the "race-free" bit alone solves real-world issues that are
> highly annoying if we don't have it.
I find it highly implausible that kdbus logging users really benefit
from knowing the *caps* of other participants in any context in which
their euid isn't enough.
>
>>>
>>> We think it's a useful and reliable authentication method. Why should
>>> we remove it?
>>
>> Because the implementation is buggy and therefore it's insecure?
>> Remember that caps are namespaced in an interesting way.
>
> Yes, we are well aware of the fact that we currently have no good way
> to translate a full set of capabilities from one user-ns to another.
> Hence, the only sane thing to do in such situations is to drop the
> entire item, which is what we do. Once we have a reliable way of
> translating things, we can add that to our code. Note that a set
> capability flag will only gain you more access level, so if caps are
> missing from a message, a user might only have *less* privileges, not
> more.
Or you could remove it in the absence of a real legitimate use case.
>
>>>
>>> Anyway, these items are just optional. The sender can refuse the
>>> reveal them, and the item is only transmitted if the receiver opted in
>>> for it, too. So there's no need to drop any item type from the
>>> protocol.
>>
>> No.
>>
>> Because if receivers opt in to most of these, *they're doing it
>> wrong*, and the kernel shouldn't be in the business of helping them.
>
> No, they are not doing it "wrong". The services would do things wrong
> if they'd make security decisions on bits that cannot be acquired in a
> race-free way. And services do things in a dirty way if they'd
> generated logging data in a race-ful way (like you suggest), by
> reading things from /proc.
Then find a clean way that's gated on having the right /proc access,
which is not guaranteed to exist on all of your eventual users'
systems, and, if that access doesn't exist because the admin or
sandbox designer has sensibly revoked it, then kdbus shouldn't
override them.
--Andy
^ permalink raw reply
* Re: [tpmdd-devel] [PATCH v7 02/10] tpm: two-phase chip management functions
From: Stefan Berger @ 2014-11-26 14:38 UTC (permalink / raw)
To: Jarkko Sakkinen, Peter Huewe, Ashley Lai, Marcel Selhorst
Cc: christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jason.gunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
trousers-tech-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <1415713513-16524-3-git-send-email-jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> tpm_register_hardware() and tpm_remove_hardware() are called often
> before initializing the device. This is wrong order since it could
> be that main TPM driver needs a fully initialized chip to be able to
> do its job. For example, now it is impossible to move common startup
> functions such as tpm_do_selftest() to tpm_register_hardware().
>
> Added tpmm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc()
> reserves memory resources and tpm_chip_register() initializes the
> device driver. This way it is possible to alter struct tpm_chip
> attributes and initialize the device driver before passing it to
> tpm_chip_register().
>
> The framework takes care of freeing struct tpm_chip by using devres
> API. The broken release callback has been wiped. For example, ACPI
> drivers do not ever get this callback.
>
> This is a interm step to get proper life-cycle for TPM device drivers.
> The next steps are adding proper ref counting and locking to tpm_chip
> that is used in every place in the TPM driver.
>
> Big thank you to Jason Gunthorpe for carefully reviewing this part
> of the code. Without his contribution reaching the best quality would
> not have been possible.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/char/tpm/Makefile | 2 +-
> drivers/char/tpm/tpm-chip.c | 196 ++++++++++++++++++++++++++++++++++++
> drivers/char/tpm/tpm-interface.c | 148 +--------------------------
> drivers/char/tpm/tpm.h | 11 +-
> drivers/char/tpm/tpm_atmel.c | 11 +-
> drivers/char/tpm/tpm_i2c_atmel.c | 33 ++----
> drivers/char/tpm/tpm_i2c_infineon.c | 37 ++-----
> drivers/char/tpm/tpm_i2c_nuvoton.c | 44 +++-----
> drivers/char/tpm/tpm_i2c_stm_st33.c | 38 +++----
> drivers/char/tpm/tpm_ibmvtpm.c | 17 ++--
> drivers/char/tpm/tpm_infineon.c | 29 +++---
> drivers/char/tpm/tpm_nsc.c | 14 ++-
> drivers/char/tpm/tpm_tis.c | 78 ++++++--------
> drivers/char/tpm/xen-tpmfront.c | 14 +--
> 14 files changed, 329 insertions(+), 343 deletions(-)
> create mode 100644 drivers/char/tpm/tpm-chip.c
>
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 4d85dd6..837da04 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for the kernel tpm device drivers.
> #
> obj-$(CONFIG_TCG_TPM) += tpm.o
> -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o
> +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o
> tpm-$(CONFIG_ACPI) += tpm_ppi.o
>
> ifdef CONFIG_ACPI
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> new file mode 100644
> index 0000000..cf3ad24
> --- /dev/null
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -0,0 +1,196 @@
> +/*
> + * Copyright (C) 2004 IBM Corporation
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * Authors:
> + * Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> + * Leendert van Doorn <leendert-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
> + * Dave Safford <safford-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
> + * Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
> + * Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *
> + * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> + *
> + * TPM chip management routines.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/freezer.h>
> +#include "tpm.h"
> +#include "tpm_eventlog.h"
> +
> +static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
> +static LIST_HEAD(tpm_chip_list);
> +static DEFINE_SPINLOCK(driver_lock);
> +
> +/*
> + * tpm_chip_find_get - return tpm_chip for a given chip number
> + * @chip_num the device number for the chip
> + */
> +struct tpm_chip *tpm_chip_find_get(int chip_num)
> +{
> + struct tpm_chip *pos, *chip = NULL;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
> + if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> + continue;
> +
> + if (try_module_get(pos->dev->driver->owner)) {
> + chip = pos;
> + break;
> + }
> + }
> + rcu_read_unlock();
> + return chip;
> +}
> +
> +/**
> + * tpmm_chip_remove() - free chip memory and device number
> + * @data: points to struct tpm_chip instance
> + *
> + * This is used internally by tpmm_chip_alloc() and called by devres
> + * when the device is released. This function does the opposite of
> + * tpmm_chip_alloc() freeing memory and the device number.
> + */
> +static void tpmm_chip_remove(void *data)
> +{
> + struct tpm_chip *chip = (struct tpm_chip *) data;
> +
> + spin_lock(&driver_lock);
> + clear_bit(chip->dev_num, dev_mask);
> + spin_unlock(&driver_lock);
> + kfree(chip);
> +}
> +
> +/**
> + * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
> + * @dev: device to which the chip is associated
> + * @ops: struct tpm_class_ops instance
> + *
> + * Allocates a new struct tpm_chip instance and assigns a free
> + * device number for it. Caller does not have to worry about
> + * freeing the allocated resources. When the devices is removed
> + * devres calls tpmm_chip_remove() to do the job.
> + */
> +struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> + const struct tpm_class_ops *ops)
> +{
> + struct tpm_chip *chip;
> +
> + chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> + if (chip == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_init(&chip->tpm_mutex);
> + INIT_LIST_HEAD(&chip->list);
> +
> + chip->ops = ops;
> +
> + spin_lock(&driver_lock);
> + chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
> + spin_unlock(&driver_lock);
> +
> + if (chip->dev_num >= TPM_NUM_DEVICES) {
> + dev_err(dev, "No available tpm device numbers\n");
> + kfree(chip);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + set_bit(chip->dev_num, dev_mask);
> +
> + scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm",
> + chip->dev_num);
nit picking here: "tpm%d", chip->dev_num would just be good enough
> +
> + chip->dev = dev;
> + devm_add_action(dev, tpmm_chip_remove, chip);
> + dev_set_drvdata(dev, chip);
> +
> + return chip;
> +}
> +EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
> +
> +/*
> + * tpm_chip_register() - create a misc driver for the TPM chip
> + * @chip: TPM chip to use.
> + *
> + * Creates a misc driver for the TPM chip and adds sysfs interfaces for
> + * the device, PPI and TCPA. As the last step this function adds the
> + * chip to the list of TPM chips available for use.
> + *
> + * NOTE: This function should be only called after the chip initialization
> + * is complete.
> + *
> + * Called from tpm_<specific>.c probe function only for devices
> + * the driver has determined it should claim. Prior to calling
> + * this function the specific probe function has called pci_enable_device
> + * upon errant exit from this function specific probe function should call
> + * pci_disable_device
> + */
> +int tpm_chip_register(struct tpm_chip *chip)
> +{
> + int rc;
> +
> + rc = tpm_dev_add_device(chip);
> + if (rc)
> + return rc;
> +
> + rc = tpm_sysfs_add_device(chip);
> + if (rc)
> + goto del_misc;
> +
> + rc = tpm_add_ppi(&chip->dev->kobj);
> + if (rc)
> + goto del_sysfs;
> +
> + chip->bios_dir = tpm_bios_log_setup(chip->devname);
> +
> + /* Make the chip available. */
> + spin_lock(&driver_lock);
> + list_add_rcu(&chip->list, &tpm_chip_list);
> + spin_unlock(&driver_lock);
> +
> + return 0;
> +del_sysfs:
> + tpm_sysfs_del_device(chip);
> +del_misc:
> + tpm_dev_del_device(chip);
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm_chip_register);
> +
> +/*
> + * tpm_chip_unregister() - release the TPM driver
> + * @chip: TPM chip to use.
> + *
> + * Takes the chip first away from the list of available TPM chips and then
> + * cleans up all the resources reserved by tpm_chip_register().
> + *
> + * NOTE: This function should be only called before deinitializing chip
> + * resources.
> + */
> +void tpm_chip_unregister(struct tpm_chip *chip)
> +{
> + spin_lock(&driver_lock);
> + list_del_rcu(&chip->list);
> + spin_unlock(&driver_lock);
> + synchronize_rcu();
> +
> + tpm_sysfs_del_device(chip);
> + tpm_remove_ppi(&chip->dev->kobj);
> +
> + if (chip->bios_dir)
> + tpm_bios_log_teardown(chip->bios_dir);
> +
> + tpm_dev_del_device(chip);
> +}
> +EXPORT_SYMBOL_GPL(tpm_chip_unregister);
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 0150b7c..915c610 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1,5 +1,6 @@
> /*
> * Copyright (C) 2004 IBM Corporation
> + * Copyright (C) 2014 Intel Corporation
> *
> * Authors:
> * Leendert van Doorn <leendert-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
> @@ -47,10 +48,6 @@ module_param_named(suspend_pcr, tpm_suspend_pcr, uint, 0644);
> MODULE_PARM_DESC(suspend_pcr,
> "PCR to use for dummy writes to faciltate flush on suspend.");
>
> -static LIST_HEAD(tpm_chip_list);
> -static DEFINE_SPINLOCK(driver_lock);
> -static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
> -
> /*
> * Array with one entry per ordinal defining the maximum amount
> * of time the chip could take to return the result. The ordinal
> @@ -639,27 +636,6 @@ static int tpm_continue_selftest(struct tpm_chip *chip)
> return rc;
> }
>
> -/*
> - * tpm_chip_find_get - return tpm_chip for given chip number
> - */
> -static struct tpm_chip *tpm_chip_find_get(int chip_num)
> -{
> - struct tpm_chip *pos, *chip = NULL;
> -
> - rcu_read_lock();
> - list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
> - if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> - continue;
> -
> - if (try_module_get(pos->dev->driver->owner)) {
> - chip = pos;
> - break;
> - }
> - }
> - rcu_read_unlock();
> - return chip;
> -}
> -
> #define TPM_ORDINAL_PCRREAD cpu_to_be32(21)
> #define READ_PCR_RESULT_SIZE 30
> static struct tpm_input_header pcrread_header = {
> @@ -887,30 +863,6 @@ again:
> }
> EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
>
> -void tpm_remove_hardware(struct device *dev)
> -{
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> -
> - if (chip == NULL) {
> - dev_err(dev, "No device data found\n");
> - return;
> - }
> -
> - spin_lock(&driver_lock);
> - list_del_rcu(&chip->list);
> - spin_unlock(&driver_lock);
> - synchronize_rcu();
> -
> - tpm_dev_del_device(chip);
> - tpm_sysfs_del_device(chip);
> - tpm_remove_ppi(&dev->kobj);
> - tpm_bios_log_teardown(chip->bios_dir);
> -
> - /* write it this way to be explicit (chip->dev == dev) */
> - put_device(chip->dev);
> -}
> -EXPORT_SYMBOL_GPL(tpm_remove_hardware);
> -
> #define TPM_ORD_SAVESTATE cpu_to_be32(152)
> #define SAVESTATE_RESULT_SIZE 10
>
> @@ -1044,104 +996,6 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
> }
> EXPORT_SYMBOL_GPL(tpm_get_random);
>
> -/* In case vendor provided release function, call it too.*/
> -
> -void tpm_dev_vendor_release(struct tpm_chip *chip)
> -{
> - if (!chip)
> - return;
> -
> - clear_bit(chip->dev_num, dev_mask);
> -}
> -EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
> -
> -
> -/*
> - * Once all references to platform device are down to 0,
> - * release all allocated structures.
> - */
> -static void tpm_dev_release(struct device *dev)
> -{
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> -
> - if (!chip)
> - return;
> -
> - tpm_dev_vendor_release(chip);
> -
> - chip->release(dev);
> - kfree(chip);
> -}
> -
> -/*
> - * Called from tpm_<specific>.c probe function only for devices
> - * the driver has determined it should claim. Prior to calling
> - * this function the specific probe function has called pci_enable_device
> - * upon errant exit from this function specific probe function should call
> - * pci_disable_device
> - */
> -struct tpm_chip *tpm_register_hardware(struct device *dev,
> - const struct tpm_class_ops *ops)
> -{
> - struct tpm_chip *chip;
> -
> - /* Driver specific per-device data */
> - chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> -
> - if (chip == NULL)
> - return NULL;
> -
> - mutex_init(&chip->tpm_mutex);
> - INIT_LIST_HEAD(&chip->list);
> -
> - chip->ops = ops;
> - chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
> -
> - if (chip->dev_num >= TPM_NUM_DEVICES) {
> - dev_err(dev, "No available tpm device numbers\n");
> - goto out_free;
> - }
> -
> - set_bit(chip->dev_num, dev_mask);
> -
> - scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm",
> - chip->dev_num);
> -
> - chip->dev = get_device(dev);
> - chip->release = dev->release;
> - dev->release = tpm_dev_release;
> - dev_set_drvdata(dev, chip);
> -
> - if (tpm_dev_add_device(chip))
> - goto put_device;
> -
> - if (tpm_sysfs_add_device(chip))
> - goto del_misc;
> -
> - if (tpm_add_ppi(&dev->kobj))
> - goto del_sysfs;
> -
> - chip->bios_dir = tpm_bios_log_setup(chip->devname);
> -
> - /* Make chip available */
> - spin_lock(&driver_lock);
> - list_add_rcu(&chip->list, &tpm_chip_list);
> - spin_unlock(&driver_lock);
> -
> - return chip;
> -
> -del_sysfs:
> - tpm_sysfs_del_device(chip);
> -del_misc:
> - tpm_dev_del_device(chip);
> -put_device:
> - put_device(chip->dev);
> -out_free:
> - kfree(chip);
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(tpm_register_hardware);
> -
> MODULE_AUTHOR("Leendert van Doorn (leendert-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org)");
> MODULE_DESCRIPTION("TPM Driver");
> MODULE_VERSION("2.0");
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index e638eb0..9880681 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -110,7 +110,6 @@ struct tpm_chip {
> struct dentry **bios_dir;
>
> struct list_head list;
> - void (*release) (struct device *);
> };
>
> #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
> @@ -322,15 +321,17 @@ extern int tpm_get_timeouts(struct tpm_chip *);
> extern void tpm_gen_interrupt(struct tpm_chip *);
> extern int tpm_do_selftest(struct tpm_chip *);
> extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
> -extern struct tpm_chip* tpm_register_hardware(struct device *,
> - const struct tpm_class_ops *ops);
> -extern void tpm_dev_vendor_release(struct tpm_chip *);
> -extern void tpm_remove_hardware(struct device *);
> extern int tpm_pm_suspend(struct device *);
> extern int tpm_pm_resume(struct device *);
> extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
> wait_queue_head_t *, bool);
>
> +struct tpm_chip *tpm_chip_find_get(int chip_num);
> +extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> + const struct tpm_class_ops *ops);
> +extern int tpm_chip_register(struct tpm_chip *chip);
> +extern void tpm_chip_unregister(struct tpm_chip *chip);
> +
> int tpm_dev_add_device(struct tpm_chip *chip);
> void tpm_dev_del_device(struct tpm_chip *chip);
> int tpm_sysfs_add_device(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 6069d13..8e2576a 100644
> --- a/drivers/char/tpm/tpm_atmel.c
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -138,11 +138,11 @@ static void atml_plat_remove(void)
> struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
>
> if (chip) {
> + tpm_chip_unregister(chip);
> if (chip->vendor.have_region)
> atmel_release_region(chip->vendor.base,
> chip->vendor.region_size);
> atmel_put_base_addr(chip->vendor.iobase);
> - tpm_remove_hardware(chip->dev);
> platform_device_unregister(pdev);
> }
> }
> @@ -184,8 +184,9 @@ static int __init init_atmel(void)
> goto err_rel_reg;
> }
>
> - if (!(chip = tpm_register_hardware(&pdev->dev, &tpm_atmel))) {
> - rc = -ENODEV;
> + chip = tpmm_chip_alloc(&pdev->dev, &tpm_atmel);
> + if (IS_ERR(chip)) {
> + rc = PTR_ERR(chip);
> goto err_unreg_dev;
> }
>
> @@ -194,6 +195,10 @@ static int __init init_atmel(void)
> chip->vendor.have_region = have_region;
> chip->vendor.region_size = region_size;
>
> + rc = tpm_chip_register(chip);
> + if (rc)
> + goto err_unreg_dev;
> +
> return 0;
>
> err_unreg_dev:
> diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
> index 7727292..8af3b4a 100644
> --- a/drivers/char/tpm/tpm_i2c_atmel.c
> +++ b/drivers/char/tpm/tpm_i2c_atmel.c
> @@ -160,11 +160,9 @@ static int i2c_atmel_probe(struct i2c_client *client,
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> return -ENODEV;
>
> - chip = tpm_register_hardware(dev, &i2c_atmel);
> - if (!chip) {
> - dev_err(dev, "%s() error in tpm_register_hardware\n", __func__);
> - return -ENODEV;
> - }
> + chip = tpmm_chip_alloc(dev, &i2c_atmel);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> chip->vendor.priv = devm_kzalloc(dev, sizeof(struct priv_data),
> GFP_KERNEL);
> @@ -179,21 +177,16 @@ static int i2c_atmel_probe(struct i2c_client *client,
> /* There is no known way to probe for this device, and all version
> * information seems to be read via TPM commands. Thus we rely on the
> * TPM startup process in the common code to detect the device. */
> - if (tpm_get_timeouts(chip)) {
> - rc = -ENODEV;
> - goto out_err;
> - }
> + if (tpm_get_timeouts(chip))
> + return -ENODEV;
>
> - if (tpm_do_selftest(chip)) {
> - rc = -ENODEV;
> - goto out_err;
> - }
> + if (tpm_do_selftest(chip))
> + return -ENODEV;
>
> - return 0;
> + rc = tpm_chip_register(chip);
> + if (rc)
> + return rc;
>
> -out_err:
> - tpm_dev_vendor_release(chip);
> - tpm_remove_hardware(chip->dev);
> return rc;
> }
>
> @@ -201,11 +194,7 @@ static int i2c_atmel_remove(struct i2c_client *client)
> {
> struct device *dev = &(client->dev);
> struct tpm_chip *chip = dev_get_drvdata(dev);
> -
> - if (chip)
> - tpm_dev_vendor_release(chip);
> - tpm_remove_hardware(dev);
> - kfree(chip);
> + tpm_chip_unregister(chip);
> return 0;
> }
>
> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
> index 472af4b..03708e6 100644
> --- a/drivers/char/tpm/tpm_i2c_infineon.c
> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> @@ -581,12 +581,9 @@ static int tpm_tis_i2c_init(struct device *dev)
> int rc = 0;
> struct tpm_chip *chip;
>
> - chip = tpm_register_hardware(dev, &tpm_tis_i2c);
> - if (!chip) {
> - dev_err(dev, "could not register hardware\n");
> - rc = -ENODEV;
> - goto out_err;
> - }
> + chip = tpmm_chip_alloc(dev, &tpm_tis_i2c);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> /* Disable interrupts */
> chip->vendor.irq = 0;
> @@ -600,7 +597,7 @@ static int tpm_tis_i2c_init(struct device *dev)
> if (request_locality(chip, 0) != 0) {
> dev_err(dev, "could not request locality\n");
> rc = -ENODEV;
> - goto out_vendor;
> + goto out_err;
> }
>
> /* read four bytes from DID_VID register */
> @@ -628,21 +625,9 @@ static int tpm_tis_i2c_init(struct device *dev)
> tpm_get_timeouts(chip);
> tpm_do_selftest(chip);
>
> - return 0;
> -
> + return tpm_chip_register(chip);
> out_release:
> release_locality(chip, chip->vendor.locality, 1);
> -
> -out_vendor:
> - /* close file handles */
> - tpm_dev_vendor_release(chip);
> -
> - /* remove hardware */
> - tpm_remove_hardware(chip->dev);
> -
> - /* reset these pointers, otherwise we oops */
> - chip->dev->release = NULL;
> - chip->release = NULL;
> tpm_dev.client = NULL;
> out_err:
> return rc;
> @@ -712,17 +697,9 @@ static int tpm_tis_i2c_probe(struct i2c_client *client,
> static int tpm_tis_i2c_remove(struct i2c_client *client)
> {
> struct tpm_chip *chip = tpm_dev.chip;
> - release_locality(chip, chip->vendor.locality, 1);
>
> - /* close file handles */
> - tpm_dev_vendor_release(chip);
> -
> - /* remove hardware */
> - tpm_remove_hardware(chip->dev);
> -
> - /* reset these pointers, otherwise we oops */
> - chip->dev->release = NULL;
> - chip->release = NULL;
> + tpm_chip_unregister(chip);
> + release_locality(chip, chip->vendor.locality, 1);
> tpm_dev.client = NULL;
>
> return 0;
> diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
> index 7b158ef..09f0c46 100644
> --- a/drivers/char/tpm/tpm_i2c_nuvoton.c
> +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
> @@ -530,11 +530,9 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
> dev_info(dev, "VID: %04X DID: %02X RID: %02X\n", (u16) vid,
> (u8) (vid >> 16), (u8) (vid >> 24));
>
> - chip = tpm_register_hardware(dev, &tpm_i2c);
> - if (!chip) {
> - dev_err(dev, "%s() error in tpm_register_hardware\n", __func__);
> - return -ENODEV;
> - }
> + chip = tpmm_chip_alloc(dev, &tpm_i2c);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> chip->vendor.priv = devm_kzalloc(dev, sizeof(struct priv_data),
> GFP_KERNEL);
> @@ -584,7 +582,7 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
> TPM_DATA_FIFO_W,
> 1, (u8 *) (&rc));
> if (rc < 0)
> - goto out_err;
> + return rc;
> /* TPM_STS <- 0x40 (commandReady) */
> i2c_nuvoton_ready(chip);
> } else {
> @@ -594,45 +592,33 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
> * only TPM_STS_VALID should be set
> */
> if (i2c_nuvoton_read_status(chip) !=
> - TPM_STS_VALID) {
> - rc = -EIO;
> - goto out_err;
> - }
> + TPM_STS_VALID)
> + return -EIO;
> }
> }
> }
>
> - if (tpm_get_timeouts(chip)) {
> - rc = -ENODEV;
> - goto out_err;
> - }
> + if (tpm_get_timeouts(chip))
> + return -ENODEV;
>
> - if (tpm_do_selftest(chip)) {
> - rc = -ENODEV;
> - goto out_err;
> - }
> + if (tpm_do_selftest(chip))
> + return -ENODEV;
>
> - return 0;
> + rc = tpm_chip_register(chip);
> + if (rc)
> + return rc;
>
> -out_err:
> - tpm_dev_vendor_release(chip);
> - tpm_remove_hardware(chip->dev);
> - return rc;
> + return 0;
> }
>
> static int i2c_nuvoton_remove(struct i2c_client *client)
> {
> struct device *dev = &(client->dev);
> struct tpm_chip *chip = dev_get_drvdata(dev);
> -
> - if (chip)
> - tpm_dev_vendor_release(chip);
> - tpm_remove_hardware(dev);
> - kfree(chip);
> + tpm_chip_unregister(chip);
> return 0;
> }
>
> -
> static const struct i2c_device_id i2c_nuvoton_id[] = {
> {I2C_DRIVER_NAME, 0},
> {}
> diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
> index 4669e37..b9d1a38 100644
> --- a/drivers/char/tpm/tpm_i2c_stm_st33.c
> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
> @@ -609,37 +609,29 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
> if (client == NULL) {
> pr_info("%s: i2c client is NULL. Device not accessible.\n",
> __func__);
> - err = -ENODEV;
> - goto end;
> + return -ENODEV;
> }
>
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> dev_info(&client->dev, "client not i2c capable\n");
> - err = -ENODEV;
> - goto end;
> + return -ENODEV;
> }
>
> - chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
> - if (!chip) {
> - dev_info(&client->dev, "fail chip\n");
> - err = -ENODEV;
> - goto end;
> - }
> + chip = tpmm_chip_alloc(&client->dev, &st_i2c_tpm);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> platform_data = client->dev.platform_data;
>
> if (!platform_data) {
> dev_info(&client->dev, "chip not available\n");
> - err = -ENODEV;
> - goto _tpm_clean_answer;
> + return -ENODEV;
> }
>
> platform_data->tpm_i2c_buffer[0] =
> kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> - if (platform_data->tpm_i2c_buffer[0] == NULL) {
> - err = -ENOMEM;
> - goto _tpm_clean_answer;
> - }
> + if (platform_data->tpm_i2c_buffer[0] == NULL)
> + return -ENOMEM;
> platform_data->tpm_i2c_buffer[1] =
> kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> if (platform_data->tpm_i2c_buffer[1] == NULL) {
> @@ -716,8 +708,10 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
> tpm_get_timeouts(chip);
> tpm_do_selftest(chip);
>
> - dev_info(chip->dev, "TPM I2C Initialized\n");
> - return 0;
> + err = tpm_chip_register(chip);
> + if (!err)
> + return 0;
> +
> _irq_set:
> free_irq(gpio_to_irq(platform_data->io_serirq), (void *)chip);
> _gpio_init2:
> @@ -732,10 +726,6 @@ _tpm_clean_response2:
> _tpm_clean_response1:
> kzfree(platform_data->tpm_i2c_buffer[0]);
> platform_data->tpm_i2c_buffer[0] = NULL;
> -_tpm_clean_answer:
> - tpm_remove_hardware(chip->dev);
> -end:
> - pr_info("TPM I2C initialisation fail\n");
> return err;
> }
>
> @@ -752,13 +742,13 @@ static int tpm_st33_i2c_remove(struct i2c_client *client)
> ((struct i2c_client *)TPM_VPRIV(chip))->dev.platform_data;
>
> if (pin_infos != NULL) {
> + tpm_chip_unregister(chip);
> +
> free_irq(pin_infos->io_serirq, chip);
>
> gpio_free(pin_infos->io_serirq);
> gpio_free(pin_infos->io_lpcpd);
>
> - tpm_remove_hardware(chip->dev);
> -
> if (pin_infos->tpm_i2c_buffer[1] != NULL) {
> kzfree(pin_infos->tpm_i2c_buffer[1]);
> pin_infos->tpm_i2c_buffer[1] = NULL;
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index af74c57..eb95796 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -270,8 +270,11 @@ static int ibmvtpm_crq_send_init(struct ibmvtpm_dev *ibmvtpm)
> static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
> {
> struct ibmvtpm_dev *ibmvtpm = ibmvtpm_get_data(&vdev->dev);
> + struct tpm_chip *chip = dev_get_drvdata(ibmvtpm->dev);
> int rc = 0;
>
> + tpm_chip_unregister(chip);
> +
> free_irq(vdev->irq, ibmvtpm);
>
> do {
> @@ -290,8 +293,6 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
> kfree(ibmvtpm->rtce_buf);
> }
>
> - tpm_remove_hardware(ibmvtpm->dev);
> -
> kfree(ibmvtpm);
>
> return 0;
> @@ -555,11 +556,9 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
> struct tpm_chip *chip;
> int rc = -ENOMEM, rc1;
>
> - chip = tpm_register_hardware(dev, &tpm_ibmvtpm);
> - if (!chip) {
> - dev_err(dev, "tpm_register_hardware failed\n");
> - return -ENODEV;
> - }
> + chip = tpmm_chip_alloc(dev, &tpm_ibmvtpm);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> ibmvtpm = kzalloc(sizeof(struct ibmvtpm_dev), GFP_KERNEL);
> if (!ibmvtpm) {
> @@ -629,7 +628,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
> if (rc)
> goto init_irq_cleanup;
>
> - return rc;
> + return tpm_chip_register(chip);
> init_irq_cleanup:
> do {
> rc1 = plpar_hcall_norets(H_FREE_CRQ, vio_dev->unit_address);
> @@ -644,8 +643,6 @@ cleanup:
> kfree(ibmvtpm);
> }
>
> - tpm_remove_hardware(dev);
> -
> return rc;
> }
>
> diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
> index dc0a255..dcdb671 100644
> --- a/drivers/char/tpm/tpm_infineon.c
> +++ b/drivers/char/tpm/tpm_infineon.c
> @@ -546,7 +546,14 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
> vendorid[0], vendorid[1],
> productid[0], productid[1], chipname);
>
> - if (!(chip = tpm_register_hardware(&dev->dev, &tpm_inf)))
> + chip = tpmm_chip_alloc(&dev->dev, &tpm_inf);
> + if (IS_ERR(chip)) {
> + rc = PTR_ERR(chip);
> + goto err_release_region;
> + }
> +
> + rc = tpm_chip_register(chip);
> + if (rc)
> goto err_release_region;
>
> return 0;
> @@ -572,17 +579,15 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
> {
> struct tpm_chip *chip = pnp_get_drvdata(dev);
>
> - if (chip) {
> - if (tpm_dev.iotype == TPM_INF_IO_PORT) {
> - release_region(tpm_dev.data_regs, tpm_dev.data_size);
> - release_region(tpm_dev.config_port,
> - tpm_dev.config_size);
> - } else {
> - iounmap(tpm_dev.mem_base);
> - release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
> - }
> - tpm_dev_vendor_release(chip);
> - tpm_remove_hardware(chip->dev);
> + tpm_chip_unregister(chip);
> +
> + if (tpm_dev.iotype == TPM_INF_IO_PORT) {
> + release_region(tpm_dev.data_regs, tpm_dev.data_size);
> + release_region(tpm_dev.config_port,
> + tpm_dev.config_size);
> + } else {
> + iounmap(tpm_dev.mem_base);
> + release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
> }
> }
>
> diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
> index 3179ec9..00c5470 100644
> --- a/drivers/char/tpm/tpm_nsc.c
> +++ b/drivers/char/tpm/tpm_nsc.c
> @@ -247,10 +247,9 @@ static struct platform_device *pdev = NULL;
> static void tpm_nsc_remove(struct device *dev)
> {
> struct tpm_chip *chip = dev_get_drvdata(dev);
> - if ( chip ) {
> - release_region(chip->vendor.base, 2);
> - tpm_remove_hardware(chip->dev);
> - }
> +
> + tpm_chip_unregister(chip);
> + release_region(chip->vendor.base, 2);
> }
>
> static SIMPLE_DEV_PM_OPS(tpm_nsc_pm, tpm_pm_suspend, tpm_pm_resume);
> @@ -308,11 +307,16 @@ static int __init init_nsc(void)
> goto err_del_dev;
> }
>
> - if (!(chip = tpm_register_hardware(&pdev->dev, &tpm_nsc))) {
> + chip = tpmm_chip_alloc(&pdev->dev, &tpm_nsc);
> + if (IS_ERR(chip)) {
> rc = -ENODEV;
> goto err_rel_reg;
> }
>
> + rc = tpm_chip_register(chip);
> + if (rc)
> + goto err_rel_reg;
> +
> dev_dbg(&pdev->dev, "NSC TPM detected\n");
> dev_dbg(&pdev->dev,
> "NSC LDN 0x%x, SID 0x%x, SRID 0x%x\n",
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 2c46734..0066b68 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -75,9 +75,6 @@ enum tis_defaults {
> #define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
> #define TPM_RID(l) (0x0F04 | ((l) << 12))
>
> -static LIST_HEAD(tis_chips);
> -static DEFINE_MUTEX(tis_lock);
> -
> #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> static int is_itpm(struct pnp_dev *dev)
> {
> @@ -528,6 +525,17 @@ static bool interrupts = true;
> module_param(interrupts, bool, 0444);
> MODULE_PARM_DESC(interrupts, "Enable interrupts");
>
> +static void tpm_tis_remove(struct tpm_chip *chip)
> +{
> + iowrite32(~TPM_GLOBAL_INT_ENABLE &
> + ioread32(chip->vendor.iobase +
> + TPM_INT_ENABLE(chip->vendor.
> + locality)),
> + chip->vendor.iobase +
> + TPM_INT_ENABLE(chip->vendor.locality));
> + release_locality(chip, chip->vendor.locality, 1);
> +}
> +
> static int tpm_tis_init(struct device *dev, resource_size_t start,
> resource_size_t len, unsigned int irq)
> {
> @@ -535,14 +543,13 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
> int rc, i, irq_s, irq_e, probe;
> struct tpm_chip *chip;
>
> - if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
> - return -ENODEV;
> + chip = tpmm_chip_alloc(dev, &tpm_tis);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> - chip->vendor.iobase = ioremap(start, len);
> - if (!chip->vendor.iobase) {
> - rc = -EIO;
> - goto out_err;
> - }
> + chip->vendor.iobase = devm_ioremap(dev, start, len);
> + if (!chip->vendor.iobase)
> + return -EIO;
>
> /* Default timeouts */
> chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> @@ -649,8 +656,8 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
> for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
> iowrite8(i, chip->vendor.iobase +
> TPM_INT_VECTOR(chip->vendor.locality));
> - if (request_irq
> - (i, tis_int_probe, IRQF_SHARED,
> + if (devm_request_irq
> + (dev, i, tis_int_probe, IRQF_SHARED,
> chip->vendor.miscdev.name, chip) != 0) {
> dev_info(chip->dev,
> "Unable to request irq: %d for probe\n",
> @@ -690,15 +697,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
> iowrite32(intmask,
> chip->vendor.iobase +
> TPM_INT_ENABLE(chip->vendor.locality));
> - free_irq(i, chip);
> }
> }
> if (chip->vendor.irq) {
> iowrite8(chip->vendor.irq,
> chip->vendor.iobase +
> TPM_INT_VECTOR(chip->vendor.locality));
> - if (request_irq
> - (chip->vendor.irq, tis_int_handler, IRQF_SHARED,
> + if (devm_request_irq
> + (dev, chip->vendor.irq, tis_int_handler, IRQF_SHARED,
> chip->vendor.miscdev.name, chip) != 0) {
> dev_info(chip->dev,
> "Unable to request irq: %d for use\n",
> @@ -719,17 +725,9 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
> }
> }
>
> - INIT_LIST_HEAD(&chip->vendor.list);
> - mutex_lock(&tis_lock);
> - list_add(&chip->vendor.list, &tis_chips);
> - mutex_unlock(&tis_lock);
> -
> -
> - return 0;
> + return tpm_chip_register(chip);
> out_err:
> - if (chip->vendor.iobase)
> - iounmap(chip->vendor.iobase);
> - tpm_remove_hardware(chip->dev);
> + tpm_tis_remove(chip);
> return rc;
> }
>
> @@ -811,13 +809,10 @@ MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
> static void tpm_tis_pnp_remove(struct pnp_dev *dev)
> {
> struct tpm_chip *chip = pnp_get_drvdata(dev);
> -
> - tpm_dev_vendor_release(chip);
> -
> - kfree(chip);
> + tpm_chip_unregister(chip);
> + tpm_tis_remove(chip);
> }
>
> -
> static struct pnp_driver tis_pnp_driver = {
> .name = "tpm_tis",
> .id_table = tpm_pnp_tbl,
> @@ -836,7 +831,7 @@ MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
>
> static struct platform_driver tis_drv = {
> .driver = {
> - .name = "tpm_tis",
> + .name = "tpm_tis",
> .owner = THIS_MODULE,
> .pm = &tpm_tis_pm,
> },
> @@ -876,31 +871,16 @@ err_dev:
>
> static void __exit cleanup_tis(void)
> {
> - struct tpm_vendor_specific *i, *j;
> struct tpm_chip *chip;
> - mutex_lock(&tis_lock);
> - list_for_each_entry_safe(i, j, &tis_chips, list) {
> - chip = to_tpm_chip(i);
> - tpm_remove_hardware(chip->dev);
> - iowrite32(~TPM_GLOBAL_INT_ENABLE &
> - ioread32(chip->vendor.iobase +
> - TPM_INT_ENABLE(chip->vendor.
> - locality)),
> - chip->vendor.iobase +
> - TPM_INT_ENABLE(chip->vendor.locality));
> - release_locality(chip, chip->vendor.locality, 1);
> - if (chip->vendor.irq)
> - free_irq(chip->vendor.irq, chip);
> - iounmap(i->iobase);
> - list_del(&i->list);
> - }
> - mutex_unlock(&tis_lock);
> #ifdef CONFIG_PNP
> if (!force) {
> pnp_unregister_driver(&tis_pnp_driver);
> return;
> }
> #endif
> + chip = dev_get_drvdata(&pdev->dev);
> + tpm_chip_unregister(chip);
> + tpm_tis_remove(chip);
> platform_device_unregister(pdev);
> platform_driver_unregister(&tis_drv);
> }
> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> index 441b44e..c3b4f5a 100644
> --- a/drivers/char/tpm/xen-tpmfront.c
> +++ b/drivers/char/tpm/xen-tpmfront.c
> @@ -175,9 +175,9 @@ static int setup_chip(struct device *dev, struct tpm_private *priv)
> {
> struct tpm_chip *chip;
>
> - chip = tpm_register_hardware(dev, &tpm_vtpm);
> - if (!chip)
> - return -ENODEV;
> + chip = tpmm_chip_alloc(dev, &tpm_vtpm);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
>
> init_waitqueue_head(&chip->vendor.read_queue);
>
> @@ -286,6 +286,7 @@ static int tpmfront_probe(struct xenbus_device *dev,
> const struct xenbus_device_id *id)
> {
> struct tpm_private *priv;
> + struct tpm_chip *chip;
> int rv;
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> @@ -302,21 +303,22 @@ static int tpmfront_probe(struct xenbus_device *dev,
>
> rv = setup_ring(dev, priv);
> if (rv) {
> - tpm_remove_hardware(&dev->dev);
> + chip = dev_get_drvdata(&dev->dev);
> + tpm_chip_unregister(chip);
> ring_free(priv);
> return rv;
> }
>
> tpm_get_timeouts(priv->chip);
>
> - return rv;
> + return tpm_chip_register(priv->chip);
> }
>
> static int tpmfront_remove(struct xenbus_device *dev)
> {
> struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
> struct tpm_private *priv = TPM_VPRIV(chip);
> - tpm_remove_hardware(&dev->dev);
> + tpm_chip_unregister(chip);
> ring_free(priv);
> TPM_VPRIV(chip) = NULL;
> return 0;
Reviewed-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
^ permalink raw reply
* Re: [PATCH 1/7] KVM: add commentary for kvm_debug_exit_arch struct
From: Andrew Jones @ 2014-11-26 14:20 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A, marc.zyngier-5wv7dgnIgG8,
peter.maydell-QSEj5FYQhm4dnm+yROfE0A, agraf-l3A5Bk7waGM,
Gleb Natapov, jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ, open list,
open list:ABI/API, dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
r65777-KZfg59tc24xl57MIdRCFDg, pbonzini-H+wXaHxf7aLQT0dZR+AlfA,
bp-l3A5Bk7waGM
In-Reply-To: <1416931805-23223-2-git-send-email-alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Tue, Nov 25, 2014 at 04:09:59PM +0000, Alex Bennée wrote:
> Bring into line with the commentary for the other structures and their
> KVM_EXIT_* cases.
>
> Signed-off-by: Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6076882..523f476 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -226,6 +226,7 @@ struct kvm_run {
> __u32 count;
> __u64 data_offset; /* relative to kvm_run start */
> } io;
> + /* KVM_EXIT_DEBUG */
> struct {
> struct kvm_debug_exit_arch arch;
> } debug;
> --
> 2.1.3
Can we change this to a 'KVM: add commentary for kvm exit type data'
patch? We're also missing
/* KVM_EXIT_INTERNAL_ERROR */
and
/* KVM_EXIT_PAPR_HCALL */
>
> _______________________________________________
> kvmarm mailing list
> kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg@public.gmane.org
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply
* Re: [tpmdd-devel] [PATCH v7 08/10] tpm: TPM 2.0 CRB Interface
From: Stefan Berger @ 2014-11-26 14:06 UTC (permalink / raw)
To: Jarkko Sakkinen, Peter Huewe, Ashley Lai, Marcel Selhorst
Cc: christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jason.gunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
trousers-tech-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <1415713513-16524-9-git-send-email-jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> tpm_crb is a driver for TPM 2.0 Command Response Buffer (CRB) Interface
> as defined in PC Client Platform TPM Profile (PTP) Specification.
>
> Only polling and single locality is supported as these are the limitations
> of the available hardware, Platform Trust Techonlogy (PTT) in Haswell
> CPUs.
>
> The driver always applies CRB with ACPI start because PTT reports using
> only ACPI start as start method but as a result of my testing it requires
> also CRB start.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/char/tpm/Kconfig | 9 ++
> drivers/char/tpm/Makefile | 1 +
> drivers/char/tpm/tpm_crb.c | 323 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 333 insertions(+)
> create mode 100644 drivers/char/tpm/tpm_crb.c
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index c54cac3..10c9419 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -122,4 +122,13 @@ config TCG_XEN
> To compile this driver as a module, choose M here; the module
> will be called xen-tpmfront.
>
> +config TCG_CRB
> + tristate "TPM 2.0 CRB Interface"
> + depends on X86 && ACPI
> + ---help---
> + If you have a TPM security chip that is compliant with the
> + TCG CRB 2.0 TPM specification say Yes and it will be accessible
> + from within Linux. To compile this driver as a module, choose
> + M here; the module will be called tpm_crb.
> +
> endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index ae56af9..e6d26dd 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
> obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
> obj-$(CONFIG_TCG_ST33_I2C) += tpm_i2c_stm_st33.o
> obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
> +obj-$(CONFIG_TCG_CRB) += tpm_crb.o
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> new file mode 100644
> index 0000000..eb221d5
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -0,0 +1,323 @@
> +/*
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * Authors:
> + * Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> + *
> + * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> + *
> + * This device driver implements the TPM interface as defined in
> + * the TCG CRB 2.0 TPM specification.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/highmem.h>
> +#include <linux/rculist.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include "tpm.h"
> +
> +#define ACPI_SIG_TPM2 "TPM2"
> +
> +static const u8 CRB_ACPI_START_UUID[] = {
> + /* 0000 */ 0xAB, 0x6C, 0xBF, 0x6B, 0x63, 0x54, 0x14, 0x47,
> + /* 0008 */ 0xB7, 0xCD, 0xF0, 0x20, 0x3C, 0x03, 0x68, 0xD4
> +};
> +
> +enum crb_defaults {
> + CRB_ACPI_START_REVISION_ID = 1,
> + CRB_ACPI_START_INDEX = 1,
> +};
> +
> +enum crb_start_method {
> + CRB_SM_ACPI_START = 2,
> + CRB_SM_CRB = 7,
> + CRB_SM_CRB_WITH_ACPI_START = 8,
> +};
> +
> +struct acpi_tpm2 {
> + struct acpi_table_header hdr;
> + u16 platform_class;
> + u16 reserved;
> + u64 control_area_pa;
> + u32 start_method;
> +};
> +
> +enum crb_ca_request {
> + CRB_CA_REQ_GO_IDLE = BIT(0),
> + CRB_CA_REQ_CMD_READY = BIT(1),
> +};
> +
> +enum crb_ca_status {
> + CRB_CA_STS_ERROR = BIT(0),
> + CRB_CA_STS_TPM_IDLE = BIT(1),
> +};
> +
> +struct crb_control_area {
> + u32 req;
> + u32 sts;
> + u32 cancel;
> + u32 start;
> + u32 int_enable;
> + u32 int_sts;
> + u32 cmd_size;
> + u64 cmd_pa;
> + u32 rsp_size;
> + u64 rsp_pa;
> +} __packed;
> +
> +enum crb_status {
> + CRB_STS_COMPLETE = BIT(0),
> +};
> +
> +enum crb_flags {
> + CRB_FL_ACPI_START = BIT(0),
> + CRB_FL_CRB_START = BIT(1),
> +};
> +
> +struct crb_priv {
> + unsigned int flags;
> + struct crb_control_area *cca;
> + unsigned long cca_pa;
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +int crb_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int crb_resume(struct device *dev)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> + (void) tpm2_do_selftest(chip);
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(crb_pm, crb_suspend, crb_resume);
> +
> +static u8 crb_status(struct tpm_chip *chip)
> +{
> + struct crb_priv *priv = chip->vendor.priv;
> + u8 sts = 0;
> +
> + if ((le32_to_cpu(priv->cca->start) & 1) != 1)
Use a #define rather than the magic '1'.
> + sts |= CRB_STS_COMPLETE;
> +
> + return sts;
> +}
> +
> +static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct crb_priv *priv = chip->vendor.priv;
> + struct crb_control_area *cca;
> + unsigned int expected;
> + unsigned long offset;
> + u8 *resp;
> +
> + cca = priv->cca;
> + if (le32_to_cpu(cca->sts) & CRB_CA_STS_ERROR)
> + return -EIO;
> +
> + offset = le64_to_cpu(cca->rsp_pa) - priv->cca_pa;
> + resp = (u8 *) ((unsigned long) cca + offset);
make sure that count >= 6?
> + memcpy(buf, resp, 6);
> + expected = be32_to_cpup((__be32 *) &buf[2]);
> +
> + if (expected > count)
> + return -EIO;
> +
> + memcpy(&buf[6], &resp[6], expected - 6);
> +
> + return expected;
> +}
> +
> +static int crb_do_acpi_start(struct tpm_chip *chip)
> +{
> + union acpi_object *obj;
> + int rc;
> +
> + obj = acpi_evaluate_dsm(chip->acpi_dev_handle,
> + CRB_ACPI_START_UUID,
> + CRB_ACPI_START_REVISION_ID,
> + CRB_ACPI_START_INDEX,
> + NULL);
> + if (!obj)
> + return -ENXIO;
> + rc = obj->integer.value == 0 ? 0 : -ENXIO;
> + ACPI_FREE(obj);
> + return rc;
> +}
> +
> +static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> + struct crb_priv *priv = chip->vendor.priv;
> + struct crb_control_area *cca;
> + u8 *cmd;
> + int rc = 0;
> +
> + cca = priv->cca;
> +
> + if (len > le32_to_cpu(cca->cmd_size)) {
> + dev_err(&chip->dev,
> + "invalid command count value %x %zx\n",
> + (unsigned int) len,
> + (size_t) le32_to_cpu(cca->cmd_size));
> + return -E2BIG;
> + }
> +
> + cmd = (u8 *) ((unsigned long) cca + le64_to_cpu(cca->cmd_pa) -
> + priv->cca_pa);
cca = priv->cca per statement above -> cmd = cca + x - cca = x
-> cmd = le64_to_cpu(cca->cmd_pa);
Should do the trick, no ?
> + memcpy(cmd, buf, len);
> +
> + /* Make sure that cmd is populated before issuing start. */
> + wmb();
> +
> + cca->start = cpu_to_le32(1);
> + rc = crb_do_acpi_start(chip);
I had commented on this already. Your TPM seems to no implement the ACPI
specs properly, or rather the ACPI table is wrong.
You have to check whether the ACPI function needs to be called. The next
TPM from a different vendor for whom the ACPI start function is not
necessary will need this check here since it will give a return code
indicating failure. Then your TPM won't work anymore! I think you should
add a check into the crb_do_acpi_start for whether this function needs
to be called or whether your TPM is being used (vendor check?) and run
this start function then anyway.
> + return rc;
> +}
> +
> +static void crb_cancel(struct tpm_chip *chip)
> +{
> + struct crb_priv *priv = chip->vendor.priv;
> + struct crb_control_area *cca;
> +
> + cca = priv->cca;
> + cca->cancel = cpu_to_le32(1);
nit: #define for this ?
> +
> + /* Make sure that cmd is populated before issuing start. */
> + wmb();
> +
> + if (crb_do_acpi_start(chip))
> + dev_err(&chip->dev, "ACPI Start failed\n");
> +
> + cca->cancel = 0;
> +}
> +
> +static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> + struct crb_priv *priv = chip->vendor.priv;
> +
> + return (le32_to_cpu(priv->cca->cancel) & 1) == 1;
> +}
> +
> +static const struct tpm_class_ops tpm_crb = {
> + .status = crb_status,
> + .recv = crb_recv,
> + .send = crb_send,
> + .cancel = crb_cancel,
> + .req_canceled = crb_req_canceled,
> + .req_complete_mask = CRB_STS_COMPLETE,
> + .req_complete_val = CRB_STS_COMPLETE,
> +};
> +
> +static int crb_acpi_add(struct acpi_device *device)
> +{
> + struct tpm_chip *chip;
> + struct acpi_tpm2 *buf;
> + struct crb_priv *priv;
> + struct device *dev = &device->dev;
> + acpi_status status;
> + u32 sm;
> + int rc;
> +
> + chip = tpmm_chip_alloc(dev, &tpm_crb);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + chip->flags = TPM_CHIP_FLAG_TPM2;
> +
> + status = acpi_get_table(ACPI_SIG_TPM2, 1,
> + (struct acpi_table_header **) &buf);
> + if (ACPI_FAILURE(status)) {
> + dev_err(dev, "failed to get TPM2 ACPI table\n");
> + return -ENODEV;
> + }
> +
> + priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
> + GFP_KERNEL);
> + if (!priv) {
> + dev_err(dev, "failed to devm_kzalloc for private data\n");
> + return -ENOMEM;
> + }
> +
> + sm = le32_to_cpu(buf->start_method);
I wonder whether you should check whether that ACPI table is big enough
to allow you accessing its start_method.
if (buf->length < sizeof(struct acpi_tpm2) ) {
return -EXYZ;
}
> +
> + if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START)
> + priv->flags |= CRB_FL_CRB_START;
You set this flag but you don't seem to check it anywhere.
> +
> + if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + priv->flags |= CRB_FL_ACPI_START;
You set this flag but you don't seem to check it anywhere.
> +
> + priv->cca_pa = le32_to_cpu(buf->control_area_pa);
> + priv->cca = (struct crb_control_area *)
> + devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
> + if (!priv->cca) {
> + dev_err(dev, "allocating memory failed\n");
> + return -ENOMEM;
> + }
> +
> + chip->vendor.priv = priv;
> +
> + /* Default timeouts and durations */
> + chip->vendor.timeout_a = usecs_to_jiffies(TPM2_TIMEOUT_A);
> + chip->vendor.timeout_b = usecs_to_jiffies(TPM2_TIMEOUT_B);
> + chip->vendor.timeout_c = usecs_to_jiffies(TPM2_TIMEOUT_C);
> + chip->vendor.timeout_d = usecs_to_jiffies(TPM2_TIMEOUT_D);
> + chip->vendor.duration[TPM_SHORT] =
> + usecs_to_jiffies(TPM2_DURATION_SHORT);
> + chip->vendor.duration[TPM_MEDIUM] =
> + usecs_to_jiffies(TPM2_DURATION_MEDIUM);
> + chip->vendor.duration[TPM_LONG] =
> + usecs_to_jiffies(TPM2_DURATION_LONG);
> +
> + chip->acpi_dev_handle = device->handle;
> +
> + rc = tpm2_do_selftest(chip);
> + if (rc)
> + return rc;
> +
> + return tpm_chip_register(chip);
> +}
> +
> +int crb_acpi_remove(struct acpi_device *device)
> +{
> + struct device *dev = &device->dev;
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> + tpm_chip_unregister(chip);
> + return 0;
> +}
> +
> +static struct acpi_device_id crb_device_ids[] = {
> + {"MSFT0101", 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, crb_device_ids);
> +
> +static struct acpi_driver crb_acpi_driver = {
> + .name = "tpm_crb",
> + .ids = crb_device_ids,
> + .ops = {
> + .add = crb_acpi_add,
> + .remove = crb_acpi_remove,
> + },
> + .drv = {
> + .pm = &crb_pm,
> + },
> +};
> +
> +module_acpi_driver(crb_acpi_driver);
> +MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>");
> +MODULE_DESCRIPTION("TPM2 Driver");
> +MODULE_VERSION("0.1");
> +MODULE_LICENSE("GPL");
Regards,
Stefan
^ permalink raw reply
* Re: futex(2) man page update help request
From: Cyril Hrubis @ 2014-11-26 13:41 UTC (permalink / raw)
To: Darren Hart
Cc: Michael Kerrisk (man-pages), Thomas Gleixner, Ingo Molnar,
Jakub Jelinek, linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
lkml, Davidlohr Bueso, Arnd Bergmann, Steven Rostedt,
Peter Zijlstra, Linux API, Carlos O'Donell
In-Reply-To: <CF9A658E.91322%dvhart-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi!
> >For this complexity of tests you would just need to call the tst_resm()
> >interface to report success/failure and, at the end of the test,
> >tst_exit() to return the stored overall test status.
> >
> >And ideally call the standard option parsing code and call the test in
> >standard loop so that the test can take advantage of standard options as
> >number of iterations to run, etc.
> >
> >Have a look at:
> >
> >https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
> >
> >there is simple test example as well as description of the interfaces.
>
>
> Thanks Cyril,
>
> I'll follow up with you in a couple weeks most likely. I have some urgent
> things that will be taking all my time and then some until then. Feel free
> to poke me though if I lose track of it :-)
Do you still plan to work on this?
--
Cyril Hrubis
chrubis-AlSwsSmVLrQ@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [tpmdd-devel] [PATCH v7 05/10] tpm: device class for tpm
From: Stefan Berger @ 2014-11-26 12:34 UTC (permalink / raw)
To: Jarkko Sakkinen, Peter Huewe, Ashley Lai, Marcel Selhorst
Cc: christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jason.gunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
trousers-tech-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <1415713513-16524-6-git-send-email-jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> Added own device class for TPM. Uses MISC_MAJOR:TPM_MINOR for the
> first character device in order to retain backwards compatability.
> Added tpm_dev_release() back attached to the character device.
> devm now only calls put_device when the platform device is removed.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/char/tpm/tpm-chip.c | 81 ++++++++++++++++++++++++++++++++++----
> drivers/char/tpm/tpm-dev.c | 36 ++---------------
> drivers/char/tpm/tpm-interface.c | 29 ++++++++++++++
> drivers/char/tpm/tpm.h | 12 ++++--
> drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
> drivers/char/tpm/tpm_tis.c | 4 +-
> 6 files changed, 116 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 96ea8eb..df40eee 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -25,6 +25,7 @@
> #include <linux/mutex.h>
> #include <linux/spinlock.h>
> #include <linux/freezer.h>
> +#include <linux/major.h>
> #include "tpm.h"
> #include "tpm_eventlog.h"
>
> @@ -32,6 +33,9 @@ static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
> static LIST_HEAD(tpm_chip_list);
> static DEFINE_SPINLOCK(driver_lock);
>
> +struct class *tpm_class;
> +dev_t tpm_devt;
> +
> /*
> * tpm_chip_find_get - return tpm_chip for a given chip number
> * @chip_num the device number for the chip
> @@ -55,16 +59,24 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
> }
>
> /**
> - * tpmm_chip_remove() - free chip memory and device number
> - * @data: points to struct tpm_chip instance
> + * tpmm_put_device() - wrap put_device() for devm
> + * @data: points to the device
> + */
> +static void tpmm_put_device(void *data)
> +{
> + struct device *dev = (struct device *) data;
> + put_device(dev);
> +}
> +
> +/**
> + * tpm_dev_release() - free chip memory and the device number
> + * @dev: the character device for the TPM chip
> *
> - * This is used internally by tpmm_chip_alloc() and called by devres
> - * when the device is released. This function does the opposite of
> - * tpmm_chip_alloc() freeing memory and the device number.
> + * This is used as the release function for the character device.
> */
> -static void tpmm_chip_remove(void *data)
> +static void tpm_dev_release(struct device *dev)
> {
> - struct tpm_chip *chip = (struct tpm_chip *) data;
> + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
>
> spin_lock(&driver_lock);
> clear_bit(chip->dev_num, dev_mask);
> @@ -112,13 +124,66 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> chip->dev_num);
>
> chip->pdev = dev;
> - devm_add_action(dev, tpmm_chip_remove, chip);
> +
> dev_set_drvdata(dev, chip);
>
> + chip->dev.class = tpm_class;
> + chip->dev.release = tpm_dev_release;
> + chip->dev.parent = chip->pdev;
> +
> + if (chip->dev_num == 0)
> + chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
> + else
> + chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
> +
> + dev_set_name(&chip->dev, chip->devname);
> +
> + device_initialize(&chip->dev);
> +
> + chip->cdev.owner = chip->pdev->driver->owner;
> + cdev_init(&chip->cdev, &tpm_fops);
> +
> + devm_add_action(dev, tpmm_put_device, chip);
> return chip;
> }
> EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
>
> +static int tpm_dev_add_device(struct tpm_chip *chip)
> +{
> + int rc;
> +
> + rc = device_add(&chip->dev);
> + if (rc) {
> + dev_err(&chip->dev,
> + "unable to device_register %s, major %d, minor %d " \
> + "err=%d\n",
> + chip->devname, MAJOR(chip->dev.devt),
> + MINOR(chip->dev.devt), rc);
> +
> + return rc;
> + }
> +
> + rc = cdev_add(&chip->cdev, chip->dev.devt, 1);
> + if (rc) {
> + dev_err(&chip->dev,
> + "unable to cdev_add %s, major %d, minor %d " \
> + "err=%d\n",
> + chip->devname, MAJOR(chip->dev.devt),
> + MINOR(chip->dev.devt), rc);
> +
> + device_unregister(&chip->dev);
> + return rc;
> + }
> +
> + return rc;
> +}
> +
> +static void tpm_dev_del_device(struct tpm_chip *chip)
> +{
> + cdev_del(&chip->cdev);
> + device_unregister(&chip->dev);
> +}
> +
> /*
> * tpm_chip_register() - create a misc driver for the TPM chip
> * @chip: TPM chip to use.
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 3568321..de0337e 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -17,7 +17,6 @@
> * License.
> *
> */
> -#include <linux/miscdevice.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include "tpm.h"
> @@ -54,9 +53,8 @@ static void timeout_work(struct work_struct *work)
>
> static int tpm_open(struct inode *inode, struct file *file)
> {
> - struct miscdevice *misc = file->private_data;
> - struct tpm_chip *chip = container_of(misc, struct tpm_chip,
> - vendor.miscdev);
> + struct tpm_chip *chip =
> + container_of(inode->i_cdev, struct tpm_chip, cdev);
> struct file_priv *priv;
>
> /* It's assured that the chip will be opened just once,
> @@ -173,7 +171,7 @@ static int tpm_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> -static const struct file_operations tpm_fops = {
> +const struct file_operations tpm_fops = {
> .owner = THIS_MODULE,
> .llseek = no_llseek,
> .open = tpm_open,
> @@ -182,32 +180,4 @@ static const struct file_operations tpm_fops = {
> .release = tpm_release,
> };
>
> -int tpm_dev_add_device(struct tpm_chip *chip)
> -{
> - int rc;
>
> - chip->vendor.miscdev.fops = &tpm_fops;
> - if (chip->dev_num == 0)
> - chip->vendor.miscdev.minor = TPM_MINOR;
> - else
> - chip->vendor.miscdev.minor = MISC_DYNAMIC_MINOR;
> -
> - chip->vendor.miscdev.name = chip->devname;
> - chip->vendor.miscdev.parent = chip->pdev;
> -
> - rc = misc_register(&chip->vendor.miscdev);
> - if (rc) {
> - chip->vendor.miscdev.name = NULL;
> - dev_err(chip->pdev,
> - "unable to misc_register %s, minor %d err=%d\n",
> - chip->vendor.miscdev.name,
> - chip->vendor.miscdev.minor, rc);
> - }
> - return rc;
> -}
> -
> -void tpm_dev_del_device(struct tpm_chip *chip)
> -{
> - if (chip->vendor.miscdev.name)
> - misc_deregister(&chip->vendor.miscdev);
> -}
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e6b08bd..9e4ce4d 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -997,6 +997,35 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
> }
> EXPORT_SYMBOL_GPL(tpm_get_random);
>
> +static int __init tpm_init(void)
> +{
> + int rc;
> +
> + tpm_class = class_create(THIS_MODULE, "tpm");
> + if (IS_ERR(tpm_class)) {
> + pr_err("couldn't create tpm class\n");
> + return PTR_ERR(tpm_class);
> + }
> +
> + rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES, "tpm");
> + if (rc < 0) {
> + pr_err("tpm: failed to allocate char dev region\n");
> + class_destroy(tpm_class);
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit tpm_exit(void)
> +{
> + class_destroy(tpm_class);
> + unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
> +}
> +
> +subsys_initcall(tpm_init);
> +module_exit(tpm_exit);
> +
> MODULE_AUTHOR("Leendert van Doorn (leendert-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org)");
> MODULE_DESCRIPTION("TPM Driver");
> MODULE_VERSION("2.0");
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b3a7c76..83103e0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -23,11 +23,11 @@
> #include <linux/fs.h>
> #include <linux/mutex.h>
> #include <linux/sched.h>
> -#include <linux/miscdevice.h>
> #include <linux/platform_device.h>
> #include <linux/io.h>
> #include <linux/tpm.h>
> #include <linux/acpi.h>
> +#include <linux/cdev.h>
>
> enum tpm_const {
> TPM_MINOR = 224, /* officially assigned */
> @@ -74,7 +74,6 @@ struct tpm_vendor_specific {
> int region_size;
> int have_region;
>
> - struct miscdevice miscdev;
> struct list_head list;
> int locality;
> unsigned long timeout_a, timeout_b, timeout_c, timeout_d; /* jiffies */
> @@ -99,6 +98,9 @@ struct tpm_vendor_specific {
>
> struct tpm_chip {
> struct device *pdev; /* Device stuff */
> + struct device dev;
> + struct cdev cdev;
> +
> const struct tpm_class_ops *ops;
>
> int dev_num; /* /dev/tpm# */
> @@ -320,6 +322,10 @@ struct tpm_cmd_t {
> tpm_cmd_params params;
> } __packed;
>
> +extern struct class *tpm_class;
> +extern dev_t tpm_devt;
> +extern const struct file_operations tpm_fops;
> +
> ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *);
> ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> size_t bufsiz);
> @@ -340,8 +346,6 @@ extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> extern int tpm_chip_register(struct tpm_chip *chip);
> extern void tpm_chip_unregister(struct tpm_chip *chip);
>
> -int tpm_dev_add_device(struct tpm_chip *chip);
> -void tpm_dev_del_device(struct tpm_chip *chip);
> int tpm_sysfs_add_device(struct tpm_chip *chip);
> void tpm_sysfs_del_device(struct tpm_chip *chip);
>
> diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
> index 92ee9fa..14246e2 100644
> --- a/drivers/char/tpm/tpm_i2c_nuvoton.c
> +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
> @@ -557,7 +557,7 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
> rc = devm_request_irq(dev, chip->vendor.irq,
> i2c_nuvoton_int_handler,
> IRQF_TRIGGER_LOW,
> - chip->vendor.miscdev.name,
> + chip->devname,
> chip);
> if (rc) {
> dev_err(dev, "%s() Unable to request irq: %d for use\n",
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 660d9af..7a2c59b 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -661,7 +661,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> TPM_INT_VECTOR(chip->vendor.locality));
> if (devm_request_irq
> (dev, i, tis_int_probe, IRQF_SHARED,
> - chip->vendor.miscdev.name, chip) != 0) {
> + chip->devname, chip) != 0) {
> dev_info(chip->pdev,
> "Unable to request irq: %d for probe\n",
> i);
> @@ -708,7 +708,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
> TPM_INT_VECTOR(chip->vendor.locality));
> if (devm_request_irq
> (dev, chip->vendor.irq, tis_int_handler, IRQF_SHARED,
> - chip->vendor.miscdev.name, chip) != 0) {
> + chip->devname, chip) != 0) {
> dev_info(chip->pdev,
> "Unable to request irq: %d for use\n",
> chip->vendor.irq);
This looks ok to me.
Reviewed-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
^ permalink raw reply
* Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support
From: One Thousand Gnomes @ 2014-11-26 12:33 UTC (permalink / raw)
To: Chunyan Zhang
Cc: mark.rutland, andrew, heiko, broonie, catalin.marinas,
will.deacon, linux-api, jslaby, artagnon, lanqing.liu, arnd,
corbet, zhang.lyra, zhizhou.zhang, geng.ren, m-karicheri2,
linux-arm-kernel, linux-serial, grant.likely, orsonzhai,
florian.vaussard, devicetree, jason, pawel.moll, ijc+devicetree,
hytszk, rrichter, broonie, wei.qiao, sprdlinux, shawn.guo, gregkh,
linux-doc, linux-kernel, robh+dt, galak
In-Reply-To: <1416917818-10506-6-git-send-email-chunyan.zhang@spreadtrum.com>
> + 213 = /dev/ttySPX0 SPRD serial port 0
> + ...
> + 216 = /dev/ttySPX3 SPRD serial port 3
Please use dynamic allocation or give a very very good reason why you
can't
> +config SERIAL_SPRD_NR
> + int "Maximum number of sprd serial ports"
> + depends on SERIAL_SPRD
> + default "4"
If you are doing dynamic allocation this should pretty much go away
> +static int sprd_startup(struct uart_port *port)
> +{
> + int ret = 0;
> + unsigned int ien, ctrl1;
> + struct sprd_uart_port *sp;
> +
> + serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
> +
> + /* clear rx fifo */
> + while (serial_in(port, SPRD_STS1) & 0x00ff)
> + serial_in(port, SPRD_RXD);
> +
> + /* clear tx fifo */
> + while (serial_in(port, SPRD_STS1) & 0xff00)
> + ;
Missing a cpu_relax and I would have thought a timeout on both of these.
> +static void sprd_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *old)
> +{
> + unsigned int baud, quot;
> + /* calculate parity */
> + lcr &= ~SPRD_LCR_PARITY;
> + if (termios->c_cflag & PARENB) {
> + lcr |= SPRD_LCR_PARITY_EN;
> + if (termios->c_cflag & PARODD)
> + lcr |= SPRD_LCR_ODD_PAR;
> + else
> + lcr |= SPRD_LCR_EVEN_PAR;
> + }
If you don't support mark/space parity then also clear CMSPAR in
termios->c_cflag. If you do then it ought to be handled above.
> +
> + /* clock divider bit16~bit20 */
> + serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
> + serial_out(port, SPRD_LCR, lcr);
> + fc |= 0x3e00 | THLD_RX_FULL;
> + serial_out(port, SPRD_CTL1, fc);
Also set the resulting baud back into the termios (see the 8250 termios
for an example)
> +static int __init sprd_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *port;
> + int baud = 115200;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (unlikely(co->index >= UART_NR_MAX || co->index < 0))
> + co->index = 0;
> +
> + port = (struct uart_port *)sprd_port[co->index];
> + if (port == NULL) {
> + pr_info("srial port %d not yet initialized\n", co->index);
Typo
Looks basically sound to me
Alan
^ permalink raw reply
* Re: [PATCH v4 0/7] kernel tinification: optionally compile out splice family of syscalls (splice, vmsplice, tee and sendfile)
From: One Thousand Gnomes @ 2014-11-26 12:19 UTC (permalink / raw)
To: David Miller
Cc: josh, rdunlap, pieter, alexander.h.duyck, viro, ast, akpm, beber,
catalina.mocanu, dborkman, edumazet, ebiederm, fabf, fuse-devel,
geert, hughd, iulia.manda21, JBeulich, bfields, jlayton,
linux-api, linux-fsdevel, linux-kernel, linux-nfs, mcgrof,
mattst88, mgorman, mst, miklos, netdev, oleg, Paul.Durrant,
paulmck, pefoley2, tgraf, therbert, trond.myklebust, willemb,
xiaoguangrong
In-Reply-To: <20141125.140441.401150380839514113.davem@davemloft.net>
On Tue, 25 Nov 2014 14:04:41 -0500 (EST)
David Miller <davem@davemloft.net> wrote:
> From: josh@joshtriplett.org
> Date: Tue, 25 Nov 2014 10:53:10 -0800
>
> > It's not a "slippery slope"; it's been our standard practice for ages.
>
> We've never put an entire class of generic system calls behind
> a config option.
Try running an original MCC Linux binary and C lib on a current kernel
We've put *entire binary formats* behind a config option. We've put older
syscalls behind it, we've put sysfs behind it, sysctl behind it, the
older microcode interfaces behind it, ISA bus as a concept behind
options. VDSO, IPC, even 32bit support ... the list goes on and on.
I'd say those were far more generic on the whole than splice/sendfile.
Alan
^ permalink raw reply
* Re: kdbus: add documentation
From: David Herrmann @ 2014-11-26 11:55 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg Kroah-Hartman, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Tom Gundersen, Jiri Kosina, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Daniel Mack,
Djalal Harouni
In-Reply-To: <CALCETrUfNG=YwwM2m78Ua4fUr9daE1omQwOSJQMKC6CTCa28fQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi
On Mon, Nov 24, 2014 at 9:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Mon, Nov 24, 2014 at 12:16 PM, David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> [snip]
>>>> +6.5 Getting information about a connection's bus creator
>>>> +--------------------------------------------------------
>>>> +
>>>> +The KDBUS_CMD_BUS_CREATOR_INFO ioctl takes the same struct as
>>>> +KDBUS_CMD_CONN_INFO but is used to retrieve information about the creator of
>>>> +the bus the connection is attached to. The metadata returned by this call is
>>>> +collected during the creation of the bus and is never altered afterwards, so
>>>> +it provides pristine information on the task that created the bus, at the
>>>> +moment when it did so.
>>>
>>> What's this for? I understand the need for the creator of busses to
>>> be authenticated, but doing it like this mean that anyone who will
>>> *fail* authentication can DoS the authentic creator.
>>
>> This returns information on a bus owner, to determine whether a
>> connection is connected to a system, user or session bus. Note that
>> the bus-creator itself is not a valid peer on the bus, so you cannot
>> send messages to them. Which kind of DoS do you have in mind?
>
> I assume that the logic is something like:
>
> connect to bus
> request bus metadata
> if (bus metadata matches expectations) {
> great, trust the bus!
> } else {
> oh crap!
> }
Uh, no, this is really not the logic that should be assumed. It's more
for code where you want to simply pass a bus fd, and the code knows
nothing about it. Now, the code can derive some information from the
bus fd, like for example who owns it. Then, depending on some of the
creds returned it can determine whether to read configuration file set
A or B and so on. This is particularly useful for all kinds of
unprivileged bus services that end up running on any kind of bus and
need to be able to figure out what they are actually operating on.
> If I'm understanding it right, then user code only really has two
> outcomes: the good case and the "oh crap!" case. The problem is that
> "oh crap!" isn't a clean failure -- if it happens, then the
> application has just been DoSed, because in that case, one of two
> things happened:
>
> 1. Some policy mismatch means that the legitimate bus owner did create
> the bus, but the user application is confused. This will result in
> difficult-to-diagnose failures.
>
> 2. A malicious or confused program created the bus. This is a DoS --
> even the legitimate bus creator can't actually create the bus now.
>
> So I think that the policy should be applied at the time that the bus
> name is claimed, not at the time that someone else tries to use the
> bus. IOW, the way that you verify you're talking to the system bus
> should be by checking that the bus is called "system", not by checking
> that UID 0 created the bus.
>
[snip]
>>>> +
>>>> +Also, if the connection allowed for file descriptor to be passed
>>>> +(KDBUS_HELLO_ACCEPT_FD), and if the message contained any, they will be
>>>> +installed into the receiving process after the KDBUS_CMD_MSG_RECV ioctl
>>>> +returns. The receiving task is obliged to close all of them appropriately.
>>>
>>> This makes it sound like fds are installed at receive time. What
>>> prevents resource exhaustion due to having excessive numbers of fds in
>>> transit (that are presumably not accounted to anyone)?
>>
>> We have a per-user message accounting for undelivered messages, as
>> well as a maximum number of pending messages per connection on the
>> receiving end. These limits are accounted on a "user<->user" basis, so
>> the limit of a user A will not affect two other users (B and C)
>> talking.
>
> But you can shove tons of fds in a message, and you can have lots of
> messages, and some of the fds can be fds of unix sockets that have fds
> queued up in them, and one of those fds could be the fd to the kdbus
> connection that sent the fd...
You cannot send kdbus-fds or unix-fds over kdbus, right now. We have
people working on the AF_UNIX gc to make it more generic and include
external types. Until then, we simply prevent recursive fd passing.
> This is not advice as to what to do about it, but I think that it will
> be a problem at some point.
>
>
>>>> +11. Policy
>>>> +===============================================================================
>>>> +
>>>> +A policy databases restrict the possibilities of connections to own, see and
>>>> +talk to well-known names. It can be associated with a bus (through a policy
>>>> +holder connection) or a custom endpoint.
>>>
>>> ISTM metadata items on bus names should be replaced with policy that
>>> applies to the domain as a whole and governs bus creation.
>>
>> No, well-known names are bound to buses, so a bus is really the right
>> place to hold policy about which process is allowed to claim them.
>> Every user is allowed to create a bus of its own, there's no policy
>> for that, and there shouldn't be.
>>
>> It has nothing to do with metadata items.
>
> But it does -- the creator of the bus binds metadata to that bus at
> creation time.
>
> I think that a better solution would be to have a global policy that
> says, for example, "to create the bus called 'system', the creator
> must have selinux label xyz" or "to create a user bus called
> uid-1000-privileged-ui-bus the creator must have some cgroup" or
> whatever.
>
> Although maybe a better solution would leave this in the kernel but
> allow any cgroup to create a bus with a same that indicates the
> creating cgroup. Then I could have my desktop shell create a
> "/cgroup/path/to/desktop" for per-user privileged things.
We enforce the UID as first entity of the bus name. Again, this is our
default policy because we rely on user-based access control. If we
want more fine-grained access-control, we can introduce other policies
at any time. For instance, we could enforce "cg-<cgroup>-<busname>"
later on, where the kernel requires the caller to prefix the bus with
"cg-<cgroup>-", where <cgroup> is the cgroup-path encoded in some way.
We provide one policy as default, and we have a use-case for it.
Further policies are always welcome as extensions later on. I don't
see why we should provide all those right from the beginning without
any users right now.
>>
>>>> +A set of policy rules is described by a name and multiple access rules, defined
>>>> +by the following struct.
>>>> +
>>>> +struct kdbus_policy_access {
>>>> + __u64 type; /* USER, GROUP, WORLD */
>>>> + One of the following.
>>>> +
>>>> + KDBUS_POLICY_ACCESS_USER
>>>> + Grant access to a user with the uid stored in the 'id' field.
>>>> +
>>>> + KDBUS_POLICY_ACCESS_GROUP
>>>> + Grant access to a user with the gid stored in the 'id' field.
>>>> +
>>>> + KDBUS_POLICY_ACCESS_WORLD
>>>> + Grant access to everyone. The 'id' field is ignored.
>>>> +
>>>> + __u64 access; /* OWN, TALK, SEE */
>>>> + The access to grant.
>>>> +
>>>> + KDBUS_POLICY_SEE
>>>> + Allow the name to be seen.
>>>> +
>>>> + KDBUS_POLICY_TALK
>>>> + Allow the name to be talked to.
>>>> +
>>>> + KDBUS_POLICY_OWN
>>>> + Allow the name to be owned.
>>>> +
>>>> + __u64 id;
>>>> + For KDBUS_POLICY_ACCESS_USER, stores the uid.
>>>> + For KDBUS_POLICY_ACCESS_GROUP, stores the gid.
>>>> +};
>>>
>>>
>>> What happens if there are multiple matches?
>>
>> We only have _granting_ policy entries. We search through the
>> policy-db until we find an entry that grants access. We do _not_ stop
>> on the first item that matches.
>
> Yay! Can you document that more clearly?
Sure!
>>
>>>
>>>> +
>>>> +11.4 TALK access and multiple well-known names per connection
>>>> +-------------------------------------------------------------
>>>> +
>>>> +Note that TALK access is checked against all names of a connection.
>>>> +For example, if a connection owns both 'org.foo.bar' and 'org.blah.baz', and
>>>> +the policy database allows 'org.blah.baz' to be talked to by WORLD, then this
>>>> +permission is also granted to 'org.foo.bar'. That might sound illogical, but
>>>> +after all, we allow messages to be directed to either the name or a well-known
>>>> +name, and policy is applied to the connection, not the name. In other words,
>>>> +the effective TALK policy for a connection is the most permissive of all names
>>>> +the connection owns.
>>>
>>> This does seem illogical. Does the recipient at least know which
>>> well-known name was addressed?
>>
>> If the sender addressed it to a well-known name, yes. If the sender
>> addressed the message to a unique-ID, there will be no such name, of
>> course. Still, the policy applies to such transactions either way
>> (standard D-Bus behavior).
>>
>> Note however that dbus1 will not pass along the destination well-known
>> name, hence most userspace libraries will ignore this information too,
>> even if they run on kdbus which might pass this information around.
>> The right way for services that carry multiple service names to
>> discern which actual service is being talked to is having separate
>> object paths for the different functionality to hide between the
>> services.
>
> It seems unfortunate to keep around this really weird behavior for the
> benefit of legacy applications. Could there perhaps be a flag that a
> connection can set to indicate that it understands per-destination
> access control and therefore wants stricter policy enforcement?
We actually think that it's a good idea not to use the destination
name for doing different things, to make things more transparent. For
example, we have tools that can explore the bus, introspect things
(like d-bus), and they should show the same objects regardless by
which name you access a service. It's more transparent then, and
really just reduces names to some labels that make addressing things
easier, but that are actually completely unnecessary for actual method
invocations.
This behaviour is also relied on by a number of current bindings. For
example GLib's implementation usually caches the unique name of a
peer, and uses that for talking to remote objects (rather than always
using the well-known name), in order to get an error back when a name
becomes unavailable (maybe because a service died) or is moved to a
different peer. If daemons would always take the destination name into
account this kind of logic could never work.
It does take some time to get used to the fact that names are
exclusively used for message routing and policies, not as
target-entity of actual method calls. But the current dbus1 behaviour
makes a ton of sense, and is really something we want to keep. It
improves how clients can do life-cycle tracking of remote objects.
Note that D-Bus modeled unique-names and well-known-names after IP
addresses and DNS names. It's a very similar model, and, like DNS
names, well-known names have no effect on the routing of messages.
>>
>>>> +11.5 Implicit policies
>>>> +----------------------
>>>> +
>>>> +Depending on the type of the endpoint, a set of implicit rules might be
>>>> +enforced. On default endpoints, the following set is enforced:
>>>> +
>>>
>>> How do these rules interact with installed policy?
>>
>> As said before, all policy entries _grant_ access. We look through all
>> entries until we find one that grants access.
>>
>>>> + * Privileged connections always override any installed policy. Those
>>>> + connections could easily install their own policies, so there is no
>>>> + reason to enforce installed policies.
>>>> + * Connections can always talk to connections of the same user. This
>>>> + includes broadcast messages.
>>>
>>> Why?
>>
>> All limits on buses are enforced on a user<->user basis. We don't want
>> to provide policies that are more fine-grained than our accounting.
>
> This seems completely at odds with all the fine-grained metadata
> stuff. Also, anything that relies on this may get very confused when
> the LSM hooks go in, because I'm reasonably sure that the intent is
> for them to *not* follow this principle.
User-based accounting has always been the default, right? We are open
to extend the API to support any other accounting scheme (LSM,
cgroup-based, ...). But like bus-name-policies, I think it's fine to
keep this as future extension. If you think the current design
precludes LSM-based accounting, lemme know and we can fix it. But we
have talked to LSM people before (and there have been patches on
LKML), and they seemed fine with it.
>>
>>> If anyone ever strengthens the concept of identity to include
>>> things other than users (hmm -- there are already groups), this could
>>> be very limiting.
>>
>> If user-based accounting is not suitable, you can create custom
>> endpoints. Future extensions to that are always welcome. So far, the
>> default user-based accounting was enough. And I think it's suitable as
>> default.
>>
>>>> + * Connections that own names might send broadcast messages to other
>>>> + connections that belong to a different user, but only if that
>>>> + destination connection does not own any name.
>>>> +
>
> (Also, what does "might" mean here?)
>
>>>
>>> This is weird. It is also differently illogical than the "illogical"
>>> thing above.
>>
>> Actually it follows the same model described above. If two connections
>> are running under the same user then broadcasts are allowed, but if
>> they are running under different users *and* if the destination owns a
>> well-known name, then broadcasts are subject to TALK policy checks
>> since that destination may own a restricted well-known name that is
>> not interested in broadcasts. So this implicit policy is just
>> fast-path for the common case where the target is subscribed to a
>> broadcast and does not own any name.
>
> Huh?
>
> Say I have two users, "Sender" and "Receiver", each with a single
> connection. If Receiver owns no well-known names, then Sender can
> send to it. If Receiver owns one well-known name, then Sender needs
> to pass a TALK check on that name. If Reciever owns two well-known
> names, then Sender only needs to pass a TALK check on one of them.
>
> Am I understanding this right? If I am, then I think this is in the
> category of baroque and inconsistent security rules which everyone
> will screw up and therefore introduce security vulnerabilities.
>
> Can you really not enforce the much simpler rule that, to send to a
> name, you must have permission to send to *that* name? If legacy
> dbus1 receivers register two names and don't validate everything
> correctly, then only the legacy receivers have problems.
Sorry, I got confused here. That implicit policy is now dropped.
>>
>>>> +
>>>> +13. Metadata
>>>> +===============================================================================
>> [snip]
>>>> +13.1 Known item types
>>>> +---------------------
>>>> +
>>>> +The following attach flags are currently supported.
>>>> +
>>>> + KDBUS_ATTACH_TIMESTAMP
>>>> + Attaches an item of type KDBUS_ITEM_TIMESTAMP which contains both the
>>>> + monotonic and the realtime timestamp, taken when the message was
>>>> + processed on the kernel side.
>>>> +
>>>> + KDBUS_ATTACH_CREDS
>>>> + Attaches an item of type KDBUS_ITEM_CREDS, containing credentials as
>>>> + described in kdbus_creds: the uid, gid, pid, tid and starttime of the task.
>>>> +
>>>
>>> As mentioned last time, please remove or justify starttime.
>>
>> starttime allows detecting PID overflows. Exposing the process
>> starttime is useful to detect when a PID is getting reused.
>> Unfortunately, we don't have 64bit pids, so we need the pid+time
>> combination to avoid ambiguity.
>
> NAK, I think.
>
> I agree that PID overflow is a real issue and should be addressed
> somehow. But please address it for real instead of adding Yet Another
> Hack (tm). In the mean time, leave that hack out, please.
>
> I would *love* to see PIDs have extra high bits at the end, done in a
> way that supports CRIU and that guarantees no reuse unless something
> privileged intentionally mis-programs it. But starttime isn't that
> mechanism.
The starttime logic sufficiently fixes the issue. If one great day in
the future somebody invents some completely new concept for making
this problem go away, we can look into that, but even then the field
is still valuable for informational purposes. I mean, the kernel
tracks this and exposes this in /proc for a reason...
In the meantime, we don't have any other way of solving this problem,
so we'll leave this in.
>>
>>>> + KDBUS_ATTACH_AUXGROUPS
>>>> + Attaches an item of type KDBUS_ITEM_AUXGROUPS, containing a dynamic
>>>> + number of auxiliary groups the sending task was a member of.
>>>> +
>>>> + KDBUS_ATTACH_NAMES
>>>> + Attaches items of type KDBUS_ITEM_OWNED_NAME, one for each name the sending
>>>> + connection currently owns. The name and flags are stored in kdbus_item.name
>>>> + for each of them.
>>>> +
>>>
>>> That's interesting. What's it for?
>>
>> It a valuable piece of information for receivers to know which bus
>> names a sender has claimed. For instance, we need this information for
>> the D-Bus proxy service, because we have to apply D-Bus1 policy in
>> that case, and we need to get a list of owned names in a race-free
>> manner to check the policy against.
>
> But if you change the rule to the sensible one where you need
> permission to TALK to the name that you're talking to, this goes away,
> right?
This does not work if a message is directed at a unique-name, as
explained above (or, think broadcasts).
>>
>>>> + KDBUS_ATTACH_TID_COMM
>>>> + Attaches an items of type KDBUS_ITEM_TID_COMM, transporting the sending
>>>> + task's 'comm', for the tid. The string is stored in kdbus_item.str.
>>>> +
>>>> + KDBUS_ATTACH_PID_COMM
>>>> + Attaches an items of type KDBUS_ITEM_PID_COMM, transporting the sending
>>>> + task's 'comm', for the pid. The string is stored in kdbus_item.str.
>>>> +
>>>> + KDBUS_ATTACH_EXE
>>>> + Attaches an item of type KDBUS_ITEM_EXE, containing the path to the
>>>> + executable of the sending task, stored in kdbus_item.str.
>>>> +
>>>> + KDBUS_ATTACH_CMDLINE
>>>> + Attaches an item of type KDBUS_ITEM_CMDLINE, containing the command line
>>>> + arguments of the sending task, as an array of strings, stored in
>>>> + kdbus_item.str.
>>>
>>> Please remove these four items. They are genuinely useless. Anything
>>> that uses them for anything is either buggy or should have asked the
>>> sender to put the value in the payload (and immediately wondered why
>>> it was doing that).
>>
>> We use them for logging, debugging and monitoring. With our wireshark
>> extension it's pretty useful to know the comm-name of a process when
>> monitoring a bus. As we explained last time, this is not about
>> security. We're aware that a process can modify them. We use them only
>> as additional meta-data for logging and debugging.
>
> Use the PID. Really. Your wireshark extention can look this crap up
> in /proc and, if it fails due to a race, big frickin' deal.
I see no reason for leaving it up to the client to do this extra work
if it can as well be attached by the kernel, really.
We use the PID on dbus1 systems for cases like this. But it's actually
too racy to be useful. For example, in systemd we ship a tiny binary
that is used as cgroup agent, which just pushes a message about the
fact it just got called for a cgroup onto the bus and then exits.
Since it only runs for a very very short time, any peer which then
tries to read the metadata off it is pretty likely to fail. And there
are quite a number of processes like that, that just do one thing and
die, especially in the early boot process. For none of them we
currently can generate useful log metadata, because all we have is a
PID that we have barely any useful information about.
This is a real race we get lots of bug-reports for. With kdbus we want
to fix this, by optionally attaching this useful data to the message,
so that the receiver can get the information when it wants to.
>>
>> If we put those items into the payload, we have to transmit this data
>> even if the destination process is not interested in this.
>> Furthermore, each caller has to run multiple syscalls on each message
>> to retrieve those values.
>>
>> We use these items heavily for filtering and debugging, regardless of
>> the payload protocol that is transmitted on the bus.
>>
>> To give another specific use-case here: dbus supports bus activation,
>> where a message sent to a non-running service causes it to be spawned
>> implicitly without losing the message. Now, with such a scheme it is
>> incredibly useful to be able to log which client caused a service to
>> be triggered, hence we want to know the cmdline/exe/comm of the
>> client. Not knowing this is a major pita when trying to trace the boot
>> process and figuring out why a specific service got activated.
>
> Again, use the PID for tracing, please.
No. It's racy. Processes can die too quickly. And this is not a race
that would be about security, but it's a real-life race that is just
awful to run into when you try to trace what's going on on your
system.
> At the very least, make it impossible to specify these fields in the
> "must be received" set and rename them to something like
> KDBUS_INSTALL_UNRELIABLE_CMDLINE, etc, because they're unreliable
>
> Finally, this stuff should only be readable by privileged users. And
> using the PID accomplishes that.
>
>>
>> Also note that since v2 of the patch there's actually a per-sender
>> mask for meta-data like this, hence a peer which doesn't want to pass
>> its exec/cmdline/comm along can do that. Of course, this will
>> seriously hamper debuggability and transparency...
>
> Transparency is a terrible thing here.
>
> How many users put passwords into things on the command line? Yes,
> it's a bad idea (for reasons that are entirely stupid), but now those
> passwords get *logged*.
>
> If this is in the kernel, and someone complains that sensitive data is
> showing up on ten different logs on their system, they'll *correctly*
> blame the kernel. If you at least use the PID and restrict it to the
> logging code, then at least the bug report will go to the logging
> daemon, which will be *correctly* accused of doing something daft, and
> it can be fixed.
Hmm? Not following here. This information is visible via /proc too. If
you hide it from /proc via the hide_pid logic, then it is also gone
from the kdbus meta-data. Also, note again that clients that don't
want this information to be passed to services can declare that now
with their sender creds mask, introduced with v2.
>>
>>>> +
>>>> + KDBUS_ATTACH_CGROUP
>>>> + Attaches an item of type KDBUS_ITEM_CGROUP with the task's cgroup path.
>>>> +
>>>> + KDBUS_ATTACH_CAPS
>>>> + Attaches an item of type KDBUS_ITEM_CAPS, carrying sets of capabilities
>>>> + that should be accessed via kdbus_item.caps.caps. Also, userspace should
>>>> + be written in a way that it takes kdbus_item.caps.last_cap into account,
>>>> + and derive the number of sets and rows from the item size and the reported
>>>> + number of valid capability bits.
>>>> +
>>>
>>> Please remove this, too, or justify its use.
>>
>> cgroup information tells us which service is doing a bus request. This
>> is useful for a variety of things. For example, the bus activation
>> logging item above benefits from it. In general, if some message shall
>> be logged about any client it is useful to know its service name.
>>
>> Capabilities are useful to authenticate specific method calls. For
>> example, when a client asks systemd to reboot, without this concept we
>> can only check for UID==0 to decide whether to allow this. Breaking
>> this down to capabilities in a race-free way has the benefit of
>> allowing systemd to bind this to CAP_SYS_BOOT instead. There is no
>> reason to deny a process with CAP_SYS_BOOT to reboot via bus-APIs, as
>> they could just enforce it via syscalls, anyway.
>
> With all due respect, BS.
>
> I admit that there is probably no reason to deny systemd-based reboot
> to a CAP_SYS_BOOT-capable process, but there is absolutely no reason
> to give processes that are supposed to reboot using systemd the
> CAP_SYS_BOOT capability.
No, and this is not how this works. Note that for unpriviliged clients
systemd checks PolicyKit in order to identify whether to allow certain
priviliged operations. However, PolicyKit requires bus chatte, is slow
and quite complex, hence the code shortcuts things if it knows that
the client is priviliged anyway, and doesn't even bother with
PolicyKit at all. This is where the caps come into play, since this
shortcutting really should *NOT* be done for every single client, but
only for those with the rights to do the operation anyway.
Hence, this is really not about overloading capabilities with new
meanings. Instead it is about shortcutting policykit for priviliged
clients. And this shortcutting should be as restricted as possible.
> In any event, I suspect you'll have a hard time justifying this for
> anything other than CAP_SYS_BOOT. Just because CAP_SYS_ADMIN users
> can probably do whatever they want doesn't mean that systemd should
> make that a built-in policy.
Well, the other option for these APIs is to use the euid, which is
hardly any better.
systemd-timedated shortcuts policykit if the client has CAP_SYS_TIME
and tries to change the system clock. Similar, if a process asks
logind to kill a session, we bypass pk if the client has CAP_KILL.
> Also, wtf is the bounding set and such for? At the very least this
> should only be the effective set.
Yes, the code that makes use of this for shortcutting pk uses the
effective set only. The other ones we allow sending across for
enhancing logging of security operations.
In general: all creds we collect at the very least are incredibly
useful for generating log records in a race-free fashion. As pointed
out above the "race-free" bit alone solves real-world issues that are
highly annoying if we don't have it.
>>
>> We think it's a useful and reliable authentication method. Why should
>> we remove it?
>
> Because the implementation is buggy and therefore it's insecure?
> Remember that caps are namespaced in an interesting way.
Yes, we are well aware of the fact that we currently have no good way
to translate a full set of capabilities from one user-ns to another.
Hence, the only sane thing to do in such situations is to drop the
entire item, which is what we do. Once we have a reliable way of
translating things, we can add that to our code. Note that a set
capability flag will only gain you more access level, so if caps are
missing from a message, a user might only have *less* privileges, not
more.
>>
>> Anyway, these items are just optional. The sender can refuse the
>> reveal them, and the item is only transmitted if the receiver opted in
>> for it, too. So there's no need to drop any item type from the
>> protocol.
>
> No.
>
> Because if receivers opt in to most of these, *they're doing it
> wrong*, and the kernel shouldn't be in the business of helping them.
No, they are not doing it "wrong". The services would do things wrong
if they'd make security decisions on bits that cannot be acquired in a
race-free way. And services do things in a dirty way if they'd
generated logging data in a race-ful way (like you suggest), by
reading things from /proc.
Thanks
David
^ permalink raw reply
* Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Tobias Klauser @ 2014-11-26 9:48 UTC (permalink / raw)
To: Chunyan Zhang
Cc: grant.likely, robh+dt, catalin.marinas, gregkh, ijc+devicetree,
jslaby, galak, broonie, mark.rutland, m-karicheri2, pawel.moll,
artagnon, rrichter, will.deacon, arnd, gnomes, corbet, jason,
broonie, heiko, shawn.guo, florian.vaussard, andrew, hytszk,
orsonzhai, geng.ren, zhizhou.zhang, lanqing.liu, zhang.lyra,
wei.qiao, devicetree, linux-arm-kernel, linux-kernel, sprdlinux,
linux-doc, linux-serial, linux-api
In-Reply-To: <1416917818-10506-6-git-send-email-chunyan.zhang@spreadtrum.com>
On 2014-11-25 at 13:16:58 +0100, Chunyan Zhang <chunyan.zhang@spreadtrum.com> wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> ---
> Documentation/devices.txt | 3 +
> drivers/tty/serial/Kconfig | 23 ++
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/sprd_serial.c | 752 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> 5 files changed, 782 insertions(+)
> create mode 100644 drivers/tty/serial/sprd_serial.c
>
[...]
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1573,6 +1573,29 @@ config SERIAL_MEN_Z135
> This driver can also be build as a module. If so, the module will be called
> men_z135_uart.ko
>
> +config SERIAL_SPRD
> + tristate "Support for SPRD serial"
> + depends on ARCH_SPRD
> + select SERIAL_CORE
> + help
> + This enables the driver for the Spreadtrum's serial.
> +
> +config SERIAL_SPRD_NR
> + int "Maximum number of sprd serial ports"
> + depends on SERIAL_SPRD
> + default "4"
> +
> +config SERIAL_SPRD_CONSOLE
> + bool "SPRD UART console support"
> + depends on SERIAL_SPRD=y
> + select SERIAL_CORE_CONSOLE
> + select SERIAL_EARLYCON
> + help
> + Support for early debug console using Spreadtrum's serial. This enables
> + the console before standard serial driver is probed. This is enabled
> + with "earlycon" on the kernel command line. The console is
> + enabled when early_param is processed.
> +
> endmenu
Please consistently use tabs instead of spaces for indentation. The help
text should be indented by one tabe + 2 spaces.
[...]
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> new file mode 100644
> index 0000000..58214c8
> --- /dev/null
> +++ b/drivers/tty/serial/sprd_serial.c
[...]
> +static inline int handle_lsr_errors(struct uart_port *port,
> + unsigned int *flag, unsigned int *lsr)
This line should be aligned with the opening ( above.
> +{
> + int ret = 0;
> +
> + /* stastics */
> + if (*lsr & SPRD_LSR_BI) {
> + *lsr &= ~(SPRD_LSR_FE | SPRD_LSR_PE);
> + port->icount.brk++;
> + ret = uart_handle_break(port);
> + if (ret)
> + return ret;
> + } else if (*lsr & SPRD_LSR_PE)
> + port->icount.parity++;
> + else if (*lsr & SPRD_LSR_FE)
> + port->icount.frame++;
> + if (*lsr & SPRD_LSR_OE)
> + port->icount.overrun++;
> +
> + /* mask off conditions which should be ignored */
> + *lsr &= port->read_status_mask;
> + if (*lsr & SPRD_LSR_BI)
> + *flag = TTY_BREAK;
> + else if (*lsr & SPRD_LSR_PE)
> + *flag = TTY_PARITY;
> + else if (*lsr & SPRD_LSR_FE)
> + *flag = TTY_FRAME;
> +
> + return ret;
> +}
> +
> +static inline void sprd_rx(int irq, void *dev_id)
> +{
> + struct uart_port *port = (struct uart_port *)dev_id;
No need to cast a void pointer.
> + struct tty_port *tty = &port->state->port;
> + unsigned int ch, flag, lsr, max_count = 2048;
> +
> + while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
> + lsr = serial_in(port, SPRD_LSR);
> + ch = serial_in(port, SPRD_RXD);
> + flag = TTY_NORMAL;
> + port->icount.rx++;
> +
> + if (unlikely(lsr & (SPRD_LSR_BI | SPRD_LSR_PE
> + | SPRD_LSR_FE | SPRD_LSR_OE)))
> + if (handle_lsr_errors(port, &lsr, &flag))
> + continue;
> + if (uart_handle_sysrq_char(port, ch))
> + continue;
> +
> + uart_insert_char(port, lsr, SPRD_LSR_OE, ch, flag);
> + }
> +
> + tty_flip_buffer_push(tty);
> +}
[...]
> +static void sprd_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *port = (struct uart_port *)sprd_port[co->index];
Better explicitly access the .port member of sprd_port[co->index] here
instead of casting.
> + int ien;
> + int locked = 1;
> +
> + if (oops_in_progress)
> + locked = spin_trylock(&port->lock);
> + else
> + spin_lock(&port->lock);
> + /* save the IEN then disable the interrupts */
> + ien = serial_in(port, SPRD_IEN);
> + serial_out(port, SPRD_IEN, 0x0);
> +
> + uart_console_write(port, s, count, sprd_console_putchar);
> +
> + /* wait for transmitter to become empty and restore the IEN */
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_IEN, ien);
> + if (locked)
> + spin_unlock(&port->lock);
> +}
> +
> +static int __init sprd_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *port;
> + int baud = 115200;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (unlikely(co->index >= UART_NR_MAX || co->index < 0))
> + co->index = 0;
> +
> + port = (struct uart_port *)sprd_port[co->index];
Same here, use the .port member of struct sprd_port[co->index].
> + if (port == NULL) {
> + pr_info("srial port %d not yet initialized\n", co->index);
Typo: should be serial instead of srial.
> + return -ENODEV;
> + }
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(port, co, baud, parity, bits, flow);
> +}
[...]
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *mem;
> + struct device_node *np = pdev->dev.of_node;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> +
> +
> + if (np)
> + pdev->id = of_alias_get_id(np, "serial");
> +
> + if (unlikely(pdev->id < 0 || pdev->id >= UART_NR_MAX)) {
> + dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
> + return -ENXIO;
> + }
> +
> + sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
> + if (!sprd_port[pdev->id])
> + return -ENOMEM;
> +
> + up = (struct uart_port *)sprd_port[pdev->id];
> + up->dev = &pdev->dev;
> + up->line = pdev->id;
> + up->type = PORT_SPRD;
> + up->iotype = SERIAL_IO_PORT;
> + up->uartclk = SPRD_DEF_RATE;
> + up->fifosize = SPRD_FIFO_SIZE;
> + up->ops = &serial_sprd_ops;
> + up->flags = ASYNC_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (unlikely(!mem)) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
> + up->mapbase = mem->start;
> + up->membase = ioremap(mem->start, resource_size(mem));
Return value of ioremap() should be checked for NULL.
> +
> + irq = platform_get_irq(pdev, 0);
> + if (unlikely(irq < 0)) {
> + dev_err(&pdev->dev, "not provide irq resource\n");
> + return -ENODEV;
> + }
> + up->irq = irq;
> +
> + platform_set_drvdata(pdev, up);
> +
> + return uart_add_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_remove(struct platform_device *dev)
> +{
> + struct uart_port *up = platform_get_drvdata(dev);
> +
> + return uart_remove_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
> + struct reg_backup *reg_bak = &(sprd_port[id]->reg_bak);
> +
> + reg_bak->ien = serial_in(port, SPRD_IEN);
> + reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
> + reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
> + reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
> + reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
> + reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
> +
> + return 0;
> +}
> +
> +static int sprd_resume(struct platform_device *dev)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
Access the .port member instead of the cast.
> + struct reg_backup *reg_bak = &(sprd_port[id]->reg_bak);
> +
> + serial_out(port, SPRD_LCR, reg_bak->ctrl0);
> + serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
> + serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
> + serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
> + serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
> + serial_out(port, SPRD_IEN, reg_bak->ien);
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH v4 09/42] virtio: set FEATURES_OK
From: Cornelia Huck @ 2014-11-26 9:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rusty, linux-api, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141125213823.GC22343@redhat.com>
On Tue, 25 Nov 2014 23:38:23 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Nov 25, 2014 at 06:48:11PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:42:01 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > set FEATURES_OK as per virtio 1.0 spec
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > include/uapi/linux/virtio_config.h | 2 ++
> > > drivers/virtio/virtio.c | 29 ++++++++++++++++++++++-------
> > > 2 files changed, 24 insertions(+), 7 deletions(-)
> > >
> >
> > > + if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > > + add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
> > > + status = dev->config->get_status(dev);
> > > + if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > > + printk(KERN_ERR "virtio: device refuses features: %x\n",
> > > + status);
> >
> > Can you use dev_err() to include the information which device failed?
>
> I'm not sure - is dev_err OK to use from probe?
The dev_* functions should be fine anytime, but the driver has already
been set before ->probe is called anyway.
^ permalink raw reply
* Re: [PATCH v3 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig
From: Arnd Bergmann @ 2014-11-26 9:00 UTC (permalink / raw)
To: Lyra Zhang
Cc: Chunyan Zhang, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Catalin Marinas,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
jslaby-AlSwsSmVLrQ@public.gmane.org, Kumar Gala, Mark Rutland,
m-karicheri2-l0cyMroinI0@public.gmane.org, Pawel Moll,
Ramkumar Ramachandra,
rrichter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org, Will Deacon,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io, corbet-T1hC0tSOHrs,
jason-NLaQJdtUoK4Be96aLqz0jA, Mark Brown,
heiko-4mtYJXux2i+zQB+pC5nmwQ, shawn.guo-KZfg59tc24xl57MIdRCFDg,
florian.vaussard-p8DiymsW2f8, andrew-g2DYL2Zd6BY,
hytszk-Re5JQEeQqe8AvxtiuMwx3w, Orson Zhai,
geng.ren-lxIno14LUO0EEoCn2XhGlw@public.gmane.org
In-Reply-To: <CAAfSe-uoXOJ8=agCFuBnXhj7nBBTskQ=8_jMtv_SAjor2gfO2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wednesday 26 November 2014 10:32:35 Lyra Zhang wrote:
>
> For now, we have only one platform(Sharkl64) based on ARM64 been submitted,
> but we're intending to add support for more our platforms based on ARM64 or
> ARM32 in the future. There are many common devices on these platforms, such
> as serial. Our idea would be that if we had a 'menuconfig ARCH_SPRD' in the
> Kconfig, these common devices only need to depend on ARCH_SPRD in the
> respective Kconfig, otherwise they may depend on a few Kconfig symbols for
> every platforms which include these common devices.
>
> So, do you think whether we should define a menuconfig(ARCH_SPRD) in the
> Kconfig for this case ?
It sounds like your other platforms are all related, so one ARCH_SPRD
should be enough. If someone wants to build a kernel that is only
going to run on a particular machine, they can just turn off all the
other drivers they don't want.
Arnd
^ permalink raw reply
* Re: [PATCH v14 0/3] Add drm driver for Rockchip Socs
From: Joerg Roedel @ 2014-11-26 7:34 UTC (permalink / raw)
To: Heiko Stübner
Cc: Dave Airlie, Mark Yao, Boris BREZILLON, Rob Clark, Daniel Vetter,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Randy Dunlap, Grant Likely, Greg Kroah-Hartman, John Stultz,
Rom Lemarchand,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA, LKML, dri-devel,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Douglas Anderson
In-Reply-To: <1557742.FTdLsE0bVs@diego>
On Wed, Nov 26, 2014 at 01:37:51AM +0100, Heiko Stübner wrote:
>
> Joerg, is your arm/rockchip branch [0] considered stable?
>
> [0] https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/log/?h=arm/rockchip
>
Yes, this branch will not be rebased. It can be pulled into another
tree.
Joerg
^ permalink raw reply
* Re: [PATCH v3 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig
From: Lyra Zhang @ 2014-11-26 3:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Chunyan Zhang, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Catalin Marinas,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
jslaby-AlSwsSmVLrQ@public.gmane.org, Kumar Gala, Mark Brown,
Mark Rutland, m-karicheri2-l0cyMroinI0@public.gmane.org,
Pawel Moll, Ramkumar Ramachandra,
rrichter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org, Will Deacon,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io, corbet-T1hC0tSOHrs,
jason-NLaQJdtUoK4Be96aLqz0jA, Mark Brown,
heiko-4mtYJXux2i+zQB+pC5nmwQ, shawn.guo-KZfg59tc24xl57MIdRCFDg,
florian.vaussard-p8DiymsW2f8, andrew-g2DYL2Zd6BY,
hytszk-Re5JQEeQqe8AvxtiuMwx3w, Orson Zhai, geng.ren
In-Reply-To: <3490516.nCDXzA8J8x@wuerfel>
2014-11-25 20:57 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
>
> On Tuesday 25 November 2014 20:16:57 Chunyan Zhang wrote:
> >
> > +menuconfig ARCH_SPRD
> > + bool "Spreadtrum SoC platform"
> > + depends on ARM64
> > + help
> > + Support for Spreadtrum ARM based SoCs
> > +
> > +if ARCH_SPRD
> > +
> > +config ARCH_SHARKL64
> > + bool "Sharkl64 SoC Platform"
> > + help
> > + Sharkl64 is a Spreadtrum's SoC Platform which is based
> > + on ARM 64-bit processor core including
> > + sc9836
> > +
> > +endif #ARCH_SPRD
> > +
>
> I don't think we need multiple levels here, it should be enough to
> have either ARCH_SPRD or ARCH_SHARKL64, because all device drivers
> are going to be optional anyway. Typically a Kconfig symbol covers
> all SoCs that are related, so if you Spreadtrum are doing both
> phone and server chips and these are designed independently, you
> would have two symbols, but if you only expect to see phone chips
> here that are all derived from the same product line, using ARCH_SPRD
> to refer to all of them should be enough.
>
> Arnd
For now, we have only one platform(Sharkl64) based on ARM64 been
submitted, but we're intending to add support for more our platforms
based on ARM64 or ARM32 in the future. There are many common devices
on these platforms, such as serial. Our idea would be that if we had a
'menuconfig ARCH_SPRD' in the Kconfig, these common devices only need
to depend on ARCH_SPRD in the respective Kconfig, otherwise they may
depend on a few Kconfig symbols for every platforms which include
these common devices.
So, do you think whether we should define a menuconfig(ARCH_SPRD) in
the Kconfig for this case ?
Thanks!
Best regards,
Chunyan
^ permalink raw reply
* Re: [tpmdd-devel] [PATCH v7 06/10] tpm: fix: move sysfs attributes to the correct place.
From: Stefan Berger @ 2014-11-26 0:48 UTC (permalink / raw)
To: Jarkko Sakkinen, Peter Huewe, Ashley Lai, Marcel Selhorst
Cc: christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jason.gunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
trousers-tech-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <1415713513-16524-7-git-send-email-jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> The sysfs attributes of the TPM device were created to the platform
> device directory that owns the character device instead of placing
> them correctly to the directory of the character device,
>
> They were also created in a racy way so that character device might
> become visible before sysfs attributes become available.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/char/tpm/tpm-chip.c | 15 ++++++---------
> drivers/char/tpm/tpm-dev.c | 2 --
> drivers/char/tpm/tpm-sysfs.c | 23 +----------------------
> drivers/char/tpm/tpm.h | 4 +---
> 4 files changed, 8 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index df40eee..5d268ac 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -29,6 +29,8 @@
> #include "tpm.h"
> #include "tpm_eventlog.h"
>
> +ATTRIBUTE_GROUPS(tpm_dev);
> +
> static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
> static LIST_HEAD(tpm_chip_list);
> static DEFINE_SPINLOCK(driver_lock);
> @@ -136,6 +138,8 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> else
> chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
>
> + chip->dev.groups = tpm_dev_groups;
> +
> dev_set_name(&chip->dev, chip->devname);
>
> device_initialize(&chip->dev);
> @@ -209,13 +213,9 @@ int tpm_chip_register(struct tpm_chip *chip)
> if (rc)
> return rc;
>
> - rc = tpm_sysfs_add_device(chip);
> - if (rc)
> - goto del_misc;
> -
> rc = tpm_add_ppi(chip);
> if (rc)
> - goto del_sysfs;
> + goto out_err;
>
> chip->bios_dir = tpm_bios_log_setup(chip->devname);
>
> @@ -225,9 +225,7 @@ int tpm_chip_register(struct tpm_chip *chip)
> spin_unlock(&driver_lock);
>
> return 0;
> -del_sysfs:
> - tpm_sysfs_del_device(chip);
> -del_misc:
> +out_err:
> tpm_dev_del_device(chip);
> return rc;
> }
> @@ -250,7 +248,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
> spin_unlock(&driver_lock);
> synchronize_rcu();
>
> - tpm_sysfs_del_device(chip);
> tpm_remove_ppi(chip);
>
> if (chip->bios_dir)
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index de0337e..f3f073f 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -179,5 +179,3 @@ const struct file_operations tpm_fops = {
> .write = tpm_write,
> .release = tpm_release,
> };
> -
> -
Unnecessary changes.
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index ee66fd4..9f5b85a 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -263,7 +263,7 @@ static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(timeouts);
>
> -static struct attribute *tpm_dev_attrs[] = {
> +struct attribute *tpm_dev_attrs[] = {
> &dev_attr_pubek.attr,
> &dev_attr_pcrs.attr,
> &dev_attr_enabled.attr,
> @@ -276,24 +276,3 @@ static struct attribute *tpm_dev_attrs[] = {
> &dev_attr_timeouts.attr,
> NULL,
> };
> -
> -static const struct attribute_group tpm_dev_group = {
> - .attrs = tpm_dev_attrs,
> -};
> -
> -int tpm_sysfs_add_device(struct tpm_chip *chip)
> -{
> - int err;
> - err = sysfs_create_group(&chip->pdev->kobj,
> - &tpm_dev_group);
> -
> - if (err)
> - dev_err(chip->pdev,
> - "failed to create sysfs attributes, %d\n", err);
> - return err;
> -}
> -
> -void tpm_sysfs_del_device(struct tpm_chip *chip)
> -{
> - sysfs_remove_group(&chip->pdev->kobj, &tpm_dev_group);
> -}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 83103e0..9d062e6 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -325,6 +325,7 @@ struct tpm_cmd_t {
> extern struct class *tpm_class;
> extern dev_t tpm_devt;
> extern const struct file_operations tpm_fops;
> +extern struct attribute *tpm_dev_attrs[];
>
> ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *);
> ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> @@ -346,9 +347,6 @@ extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> extern int tpm_chip_register(struct tpm_chip *chip);
> extern void tpm_chip_unregister(struct tpm_chip *chip);
>
> -int tpm_sysfs_add_device(struct tpm_chip *chip);
> -void tpm_sysfs_del_device(struct tpm_chip *chip);
> -
> int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
>
> #ifdef CONFIG_ACPI
Other than those stray line deletions, it looks good to me.
Stefan
^ permalink raw reply
* Re: [tpmdd-devel] [PATCH v7 07/10] tpm: TPM 2.0 baseline support
From: Stefan Berger @ 2014-11-26 0:42 UTC (permalink / raw)
To: Jarkko Sakkinen, Peter Huewe, Ashley Lai, Marcel Selhorst
Cc: christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Will Arthur,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jason.gunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
trousers-tech-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <1415713513-16524-8-git-send-email-jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> TPM 2.0 devices are separated by adding a field 'flags' to struct
> tpm_chip and defining a flag TPM_CHIP_FLAG_TPM2 for tagging them.
>
> This patch adds the following internal functions:
>
> - tpm2_get_random()
> - tpm2_get_tpm_pt()
> - tpm2_pcr_extend()
> - tpm2_pcr_read()
> - tpm2_startup()
>
> Additionally, the following exported functions are implemented for
> implementing TPM 2.0 device drivers:
>
> - tpm2_do_selftest()
> - tpm2_calc_ordinal_durations()
> - tpm2_gen_interrupt()
>
> The existing functions that are exported for the use for existing
> subsystems have been changed to check the flags field in struct
> tpm_chip and use appropriate TPM 2.0 counterpart if
> TPM_CHIP_FLAG_TPM2 is est.
>
> The code for tpm2_calc_ordinal_duration() and tpm2_startup() were
> originally written by Will Arthur.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Will Arthur <will.c.arthur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> drivers/char/tpm/Makefile | 2 +-
> drivers/char/tpm/tpm-chip.c | 21 +-
> drivers/char/tpm/tpm-interface.c | 24 +-
> drivers/char/tpm/tpm.h | 67 +++++
> drivers/char/tpm/tpm2-cmd.c | 566 +++++++++++++++++++++++++++++++++++++++
> 5 files changed, 668 insertions(+), 12 deletions(-)
> create mode 100644 drivers/char/tpm/tpm2-cmd.c
>
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 837da04..ae56af9 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for the kernel tpm device drivers.
> #
> obj-$(CONFIG_TCG_TPM) += tpm.o
> -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o
> +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
> tpm-$(CONFIG_ACPI) += tpm_ppi.o
>
> ifdef CONFIG_ACPI
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 5d268ac..4d25b24 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -213,11 +213,14 @@ int tpm_chip_register(struct tpm_chip *chip)
> if (rc)
> return rc;
>
> - rc = tpm_add_ppi(chip);
> - if (rc)
> - goto out_err;
> + /* Populate sysfs for TPM1 devices. */
> + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> + rc = tpm_add_ppi(chip);
> + if (rc)
> + goto out_err;
>
> - chip->bios_dir = tpm_bios_log_setup(chip->devname);
> + chip->bios_dir = tpm_bios_log_setup(chip->devname);
> + }
>
> /* Make the chip available. */
> spin_lock(&driver_lock);
> @@ -248,10 +251,12 @@ void tpm_chip_unregister(struct tpm_chip *chip)
> spin_unlock(&driver_lock);
> synchronize_rcu();
>
> - tpm_remove_ppi(chip);
> -
> - if (chip->bios_dir)
> - tpm_bios_log_teardown(chip->bios_dir);
> + /* Clean up sysfs for TPM1 devices. */
> + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> + if (chip->bios_dir)
> + tpm_bios_log_teardown(chip->bios_dir);
> + tpm_remove_ppi(chip);
> + }
>
> tpm_dev_del_device(chip);
> }
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 9e4ce4d..e62b835 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -360,7 +360,10 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> if (chip->vendor.irq)
> goto out_recv;
>
> - stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + stop = jiffies + tpm2_calc_ordinal_duration(chip, ordinal);
> + else
> + stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> do {
> u8 status = chip->ops->status(chip);
> if ((status & chip->ops->req_complete_mask) ==
> @@ -483,7 +486,7 @@ static const struct tpm_input_header tpm_startup_header = {
> static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
> {
> struct tpm_cmd_t start_cmd;
> - start_cmd.header.in = tpm_startup_header;
> +
> start_cmd.params.startup_in.startup_type = startup_type;
> return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE,
> "attempting to start the TPM");
> @@ -680,7 +683,10 @@ int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
> chip = tpm_chip_find_get(chip_num);
> if (chip == NULL)
> return -ENODEV;
> - rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
> + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
> + else
> + rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
> tpm_chip_put(chip);
> return rc;
> }
> @@ -714,6 +720,12 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> if (chip == NULL)
> return -ENODEV;
>
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + rc = tpm2_pcr_extend(chip, pcr_idx, hash);
> + tpm_chip_put(chip);
> + return rc;
> + }
> +
> cmd.header.in = pcrextend_header;
> cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
> memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE);
> @@ -974,6 +986,12 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
> if (chip == NULL)
> return -ENODEV;
>
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + err = tpm2_get_random(chip, out, max);
> + tpm_chip_put(chip);
> + return err;
> + }
> +
> do {
> tpm_cmd.header.in = tpm_getrandom_header;
> tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 9d062e6..8a434d2 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -62,6 +62,57 @@ enum tpm_duration {
> #define TPM_ERR_INVALID_POSTINIT 38
>
> #define TPM_HEADER_SIZE 10
> +
> +enum tpm2_const {
> + TPM2_PLATFORM_PCR = 24,
> + TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
> + TPM2_TIMEOUT_A = 750 * 1000,
> + TPM2_TIMEOUT_B = 2000 * 1000,
> + TPM2_TIMEOUT_C = 200 * 1000,
> + TPM2_TIMEOUT_D = 30 * 1000,
> + TPM2_DURATION_SHORT = 20 * 1000,
> + TPM2_DURATION_MEDIUM = 750 * 1000,
> + TPM2_DURATION_LONG = 2000 * 1000,
> +};
> +
> +enum tpm2_structures {
> + TPM2_ST_NO_SESSIONS = 0x8001,
> + TPM2_ST_SESSIONS = 0x8002,
> +};
> +
> +enum tpm2_return_codes {
> + TPM2_RC_TESTING = 0x090A,
> + TPM2_RC_DISABLED = 0x0120,
> +};
> +
> +enum tpm2_algorithms {
> + TPM2_ALG_SHA1 = 0x0004,
> +};
> +
> +enum tpm2_command_codes {
> + TPM2_CC_FIRST = 0x011F,
> + TPM2_CC_SELF_TEST = 0x0143,
> + TPM2_CC_STARTUP = 0x0144,
> + TPM2_CC_GET_CAPABILITY = 0x017A,
> + TPM2_CC_GET_RANDOM = 0x017B,
> + TPM2_CC_PCR_READ = 0x017E,
> + TPM2_CC_PCR_EXTEND = 0x0182,
> + TPM2_CC_LAST = 0x018F,
> +};
> +
> +enum tpm2_permanent_handles {
> + TPM2_RS_PW = 0x40000009,
> +};
> +
> +enum tpm2_capabilities {
> + TPM2_CAP_TPM_PROPERTIES = 6,
> +};
> +
> +enum tpm2_startup_types {
> + TPM2_SU_CLEAR = 0x0000,
> + TPM2_SU_STATE = 0x0001,
> +};
> +
> struct tpm_chip;
>
> struct tpm_vendor_specific {
> @@ -96,12 +147,17 @@ struct tpm_vendor_specific {
>
> #define TPM_PPI_VERSION_LEN 3
>
> +enum tpm_chip_flags {
> + TPM_CHIP_FLAG_TPM2 = BIT(0),
> +};
> +
> struct tpm_chip {
> struct device *pdev; /* Device stuff */
> struct device dev;
> struct cdev cdev;
>
> const struct tpm_class_ops *ops;
> + unsigned int flags;
>
> int dev_num; /* /dev/tpm# */
> char devname[7];
> @@ -362,3 +418,14 @@ static inline void tpm_remove_ppi(struct tpm_chip *chip)
> {
> }
> #endif
> +
> +int tpm2_startup(struct tpm_chip *chip, __be16 startup_type);
> +int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
> +int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
> +int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max);
> +
> +extern ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
> + u32 *value, const char *desc);
> +extern unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *, u32);
> +extern int tpm2_do_selftest(struct tpm_chip *chip);
> +extern int tpm2_gen_interrupt(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> new file mode 100644
> index 0000000..458a17d
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -0,0 +1,566 @@
> +/*
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * Authors:
> + * Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> + *
> + * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> + *
> + * This file contains TPM2 protocol implementations of the commands
> + * used by the kernel internally.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include "tpm.h"
> +
> +struct tpm2_startup_in {
> + __be16 startup_type;
> +} __packed;
> +
> +struct tpm2_self_test_in {
> + u8 full_test;
> +} __packed;
> +
> +struct tpm2_pcr_read_in {
> + __be32 pcr_selects_cnt;
> + __be16 hash_alg;
> + u8 pcr_select_size;
> + u8 pcr_select[TPM2_PCR_SELECT_MIN];
> +} __packed;
> +
> +struct tpm2_pcr_read_out {
> + __be32 update_cnt;
> + __be32 pcr_selects_cnt;
> + __be16 hash_alg;
> + u8 pcr_select_size;
> + u8 pcr_select[TPM2_PCR_SELECT_MIN];
> + __be32 digests_cnt;
> + __be16 digest_size;
> + u8 digest[TPM_DIGEST_SIZE];
> +} __packed;
> +
> +struct tpm2_null_auth_area {
> + __be32 handle;
> + __be16 nonce_size;
> + u8 attributes;
> + __be16 auth_size;
> +} __packed;
> +
> +struct tpm2_pcr_extend_in {
> + __be32 pcr_idx;
> + __be32 auth_area_size;
> + struct tpm2_null_auth_area auth_area;
> + __be32 digest_cnt;
> + __be16 hash_alg;
> + u8 digest[TPM_DIGEST_SIZE];
> +} __packed;
> +
> +struct tpm2_get_tpm_pt_in {
> + __be32 cap_id;
> + __be32 property_id;
> + __be32 property_cnt;
> +} __packed;
> +
> +struct tpm2_get_tpm_pt_out {
> + u8 more_data;
> + __be32 subcap_id;
> + __be32 property_cnt;
> + __be32 property_id;
> + __be32 value;
> +} __packed;
> +
> +struct tpm2_get_random_in {
> + __be16 size;
> +} __packed;
> +
> +struct tpm2_get_random_out {
> + __be16 size;
> + u8 buffer[TPM_MAX_RNG_DATA];
> +} __packed;
> +
> +union tpm2_cmd_params {
> + struct tpm2_startup_in startup_in;
> + struct tpm2_self_test_in selftest_in;
> + struct tpm2_pcr_read_in pcrread_in;
> + struct tpm2_pcr_read_out pcrread_out;
> + struct tpm2_pcr_extend_in pcrextend_in;
> + struct tpm2_get_tpm_pt_in get_tpm_pt_in;
> + struct tpm2_get_tpm_pt_out get_tpm_pt_out;
> + struct tpm2_get_random_in getrandom_in;
> + struct tpm2_get_random_out getrandom_out;
> +};
> +
> +struct tpm2_cmd {
> + tpm_cmd_header header;
> + union tpm2_cmd_params params;
> +} __packed;
> +
> +/*
> + * Array with one entry per ordinal defining the maximum amount
> + * of time the chip could take to return the result. The values
> + * of the SHORT, MEDIUM, and LONG durations are taken from the
> + * PC Client Profile (PTP) specification.
> + */
> +static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
> + TPM_UNDEFINED, /* 11F */
> + TPM_UNDEFINED, /* 120 */
> + TPM_LONG, /* 121 */
> + TPM_UNDEFINED, /* 122 */
> + TPM_UNDEFINED, /* 123 */
> + TPM_UNDEFINED, /* 124 */
> + TPM_UNDEFINED, /* 125 */
> + TPM_UNDEFINED, /* 126 */
> + TPM_UNDEFINED, /* 127 */
> + TPM_UNDEFINED, /* 128 */
> + TPM_LONG, /* 129 */
> + TPM_UNDEFINED, /* 12a */
> + TPM_UNDEFINED, /* 12b */
> + TPM_UNDEFINED, /* 12c */
> + TPM_UNDEFINED, /* 12d */
> + TPM_UNDEFINED, /* 12e */
> + TPM_UNDEFINED, /* 12f */
> + TPM_UNDEFINED, /* 130 */
> + TPM_UNDEFINED, /* 131 */
> + TPM_UNDEFINED, /* 132 */
> + TPM_UNDEFINED, /* 133 */
> + TPM_UNDEFINED, /* 134 */
> + TPM_UNDEFINED, /* 135 */
> + TPM_UNDEFINED, /* 136 */
> + TPM_UNDEFINED, /* 137 */
> + TPM_UNDEFINED, /* 138 */
> + TPM_UNDEFINED, /* 139 */
> + TPM_UNDEFINED, /* 13a */
> + TPM_UNDEFINED, /* 13b */
> + TPM_UNDEFINED, /* 13c */
> + TPM_UNDEFINED, /* 13d */
> + TPM_MEDIUM, /* 13e */
> + TPM_UNDEFINED, /* 13f */
> + TPM_UNDEFINED, /* 140 */
> + TPM_UNDEFINED, /* 141 */
> + TPM_UNDEFINED, /* 142 */
> + TPM_LONG, /* 143 */
> + TPM_MEDIUM, /* 144 */
> + TPM_UNDEFINED, /* 145 */
> + TPM_UNDEFINED, /* 146 */
> + TPM_UNDEFINED, /* 147 */
> + TPM_UNDEFINED, /* 148 */
> + TPM_UNDEFINED, /* 149 */
> + TPM_UNDEFINED, /* 14a */
> + TPM_UNDEFINED, /* 14b */
> + TPM_UNDEFINED, /* 14c */
> + TPM_UNDEFINED, /* 14d */
> + TPM_LONG, /* 14e */
> + TPM_UNDEFINED, /* 14f */
> + TPM_UNDEFINED, /* 150 */
> + TPM_UNDEFINED, /* 151 */
> + TPM_UNDEFINED, /* 152 */
> + TPM_UNDEFINED, /* 153 */
> + TPM_UNDEFINED, /* 154 */
> + TPM_UNDEFINED, /* 155 */
> + TPM_UNDEFINED, /* 156 */
> + TPM_UNDEFINED, /* 157 */
> + TPM_UNDEFINED, /* 158 */
> + TPM_UNDEFINED, /* 159 */
> + TPM_UNDEFINED, /* 15a */
> + TPM_UNDEFINED, /* 15b */
> + TPM_MEDIUM, /* 15c */
> + TPM_UNDEFINED, /* 15d */
> + TPM_UNDEFINED, /* 15e */
> + TPM_UNDEFINED, /* 15f */
> + TPM_UNDEFINED, /* 160 */
> + TPM_UNDEFINED, /* 161 */
> + TPM_UNDEFINED, /* 162 */
> + TPM_UNDEFINED, /* 163 */
> + TPM_UNDEFINED, /* 164 */
> + TPM_UNDEFINED, /* 165 */
> + TPM_UNDEFINED, /* 166 */
> + TPM_UNDEFINED, /* 167 */
> + TPM_UNDEFINED, /* 168 */
> + TPM_UNDEFINED, /* 169 */
> + TPM_UNDEFINED, /* 16a */
> + TPM_UNDEFINED, /* 16b */
> + TPM_UNDEFINED, /* 16c */
> + TPM_UNDEFINED, /* 16d */
> + TPM_UNDEFINED, /* 16e */
> + TPM_UNDEFINED, /* 16f */
> + TPM_UNDEFINED, /* 170 */
> + TPM_UNDEFINED, /* 171 */
> + TPM_UNDEFINED, /* 172 */
> + TPM_UNDEFINED, /* 173 */
> + TPM_UNDEFINED, /* 174 */
> + TPM_UNDEFINED, /* 175 */
> + TPM_UNDEFINED, /* 176 */
> + TPM_LONG, /* 177 */
> + TPM_UNDEFINED, /* 178 */
> + TPM_UNDEFINED, /* 179 */
> + TPM_MEDIUM, /* 17a */
> + TPM_LONG, /* 17b */
> + TPM_UNDEFINED, /* 17c */
> + TPM_UNDEFINED, /* 17d */
> + TPM_UNDEFINED, /* 17e */
> + TPM_UNDEFINED, /* 17f */
> + TPM_UNDEFINED, /* 180 */
> + TPM_UNDEFINED, /* 181 */
> + TPM_MEDIUM, /* 182 */
> + TPM_UNDEFINED, /* 183 */
> + TPM_UNDEFINED, /* 184 */
> + TPM_MEDIUM, /* 185 */
> + TPM_MEDIUM, /* 186 */
> + TPM_UNDEFINED, /* 187 */
> + TPM_UNDEFINED, /* 188 */
> + TPM_UNDEFINED, /* 189 */
> + TPM_UNDEFINED, /* 18a */
> + TPM_UNDEFINED, /* 18b */
> + TPM_UNDEFINED, /* 18c */
> + TPM_UNDEFINED, /* 18d */
> + TPM_UNDEFINED, /* 18e */
> + TPM_UNDEFINED /* 18f */
> +};
> +
> +static const struct tpm_input_header tpm2_startup_header = {
> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
> + .length = cpu_to_be32(12),
12 -> sizeof(struct tpm_input_header) + sizeof(struct pm2_startup_in)
> + .ordinal = cpu_to_be32(TPM2_CC_STARTUP)
> +};
> +
> +/**
> + * tpm2_startup() - send startup command to the TPM chip
> + * @chip: TPM chip to use.
> + * @startup_type startup type. The value is either
> + * TPM_SU_CLEAR or TPM_SU_STATE.
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
Replace 'When's with 'If's. (when being 'temporal')
> + */
> +int tpm2_startup(struct tpm_chip *chip, __be16 startup_type)
> +{
> + struct tpm2_cmd cmd;
> +
> + cmd.header.in = tpm2_startup_header;
> +
> + cmd.params.startup_in.startup_type = startup_type;
> + return tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
> + "attempting to start the TPM");
> +}
> +
> +#define TPM2_PCR_READ_IN_SIZE \
> + (sizeof(struct tpm_input_header) + \
> + sizeof(struct tpm2_pcr_read_in))
> +
Ah! You could also use a #define above!
> +static const struct tpm_input_header tpm2_pcrread_header = {
> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
> + .length = cpu_to_be32(TPM2_PCR_READ_IN_SIZE),
> + .ordinal = cpu_to_be32(TPM2_CC_PCR_READ)
> +};
> +
> +/**
> + * tpm2_pcr_read() - read a PCR value
> + * @chip: TPM chip to use.
> + * @pcr_idx: index of the PCR to read.
> + * @ref_buf: buffer to store the resulting hash,
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
Replace 'When's with 'If's. Also further below.
> +int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
> +{
> + int rc;
> + struct tpm2_cmd cmd;
> + u8 *buf;
> + int i, j;
> +
> + if (pcr_idx >= TPM2_PLATFORM_PCR)
> + return -EINVAL;
> +
> + cmd.header.in = tpm2_pcrread_header;
> + cmd.params.pcrread_in.pcr_selects_cnt = cpu_to_be32(1);
> + cmd.params.pcrread_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
> + cmd.params.pcrread_in.pcr_select_size = TPM2_PCR_SELECT_MIN;
> +
> + for (i = 0; i < TPM2_PCR_SELECT_MIN; i++) {
> + j = pcr_idx - i * 8;
> +
> + cmd.params.pcrread_in.pcr_select[i] =
> + (j >= 0 && j < 8) ? 1 << j : 0;
> + }
Umpf - what's this? You need to set the PCR index as an index in the
bitfield?
pcr_idx >> 3 gives you the index into the array, assuming that [0]
holds bits for PCR0 to 7.
1 << (pcr_idx & 0x7) gives you the bit to set, assuming bit 0 is to be
set for PCR 0
1 << (7- (pcr_idx & 0x7)) gives you the bit to set, assuming bit 7 is to
be set for PCR 0.
memset(cmd.params.pcrread_in.pcr_select, 0,
sizeof(cmd.params.pcrread_in.pcr_select));
cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
> +
> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
> + "attempting to read a pcr value");
> +
> + if (rc == 0) {
> + buf = cmd.params.pcrread_out.digest;
> + memcpy(res_buf, buf, TPM_DIGEST_SIZE);
> + }
> +
> + return rc;
> +}
> +
> +/**
> + * tpm2_pcr_extend() - extend a PCR value
> + * @chip: TPM chip to use.
> + * @pcr_idx: index of the PCR.
> + * @hash: hash value to use for the extend operation.
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +static const struct tpm_input_header tpm2_pcrextend_header = {
> + .tag = cpu_to_be16(TPM2_ST_SESSIONS),
> + .length = cpu_to_be32(sizeof(struct tpm_input_header) +
> + sizeof(struct tpm2_pcr_extend_in)),
> + .ordinal = cpu_to_be32(TPM2_CC_PCR_EXTEND)
> +};
> +
> +int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> +{
> + struct tpm2_cmd cmd;
> + int rc;
> +
> + cmd.header.in = tpm2_pcrextend_header;
> + cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
> + cmd.params.pcrextend_in.auth_area_size =
> + cpu_to_be32(sizeof(struct tpm2_null_auth_area));
> + cmd.params.pcrextend_in.auth_area.handle =
> + cpu_to_be32(TPM2_RS_PW);
> + cmd.params.pcrextend_in.auth_area.nonce_size = 0;
> + cmd.params.pcrextend_in.auth_area.attributes = 0;
> + cmd.params.pcrextend_in.auth_area.auth_size = 0;
> + cmd.params.pcrextend_in.digest_cnt = cpu_to_be32(1);
> + cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
> + memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE);
> +
> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
> + "attempting extend a PCR value");
> +
> + return rc;
> +}
> +
> +static const struct tpm_input_header tpm2_getrandom_header = {
> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
> + .length = cpu_to_be32(sizeof(struct tpm_input_header) +
> + sizeof(struct tpm2_get_random_in)),
> + .ordinal = cpu_to_be32(TPM2_CC_GET_RANDOM)
> +};
> +
> +/**
> + * tpm2_get_random() - get random bytes from the TPM RNG
> + * @chip: TPM chip to use
> + * @out: destination buffer for the random bytes
> + * @max: the max number of bytes to write to @out
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
> +{
> + struct tpm2_cmd cmd;
> + u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA);
num_bytes = min_t(u32, max, sizeof(tpm2_cmd.params.getrandom_out.buffer);
This way you tie it to the actual buffer size of the buffer the TPM can fill
> + int err, total = 0, retries = 5;
> + u8 *dest = out;
> +
> + if (!out || !num_bytes || max > TPM_MAX_RNG_DATA)
... || max > sizeof((tpm2_cmd.params.getrandom_out.buffer)
> + return -EINVAL;
> +
> + do {
> + cmd.header.in = tpm2_getrandom_header;
> + cmd.params.getrandom_in.size = cpu_to_be16(num_bytes);
> +
> + err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
> + "attempting get random");
> + if (err)
> + break;
> +
> + recd = be16_to_cpu(cmd.params.getrandom_out.size);
> + memcpy(dest, cmd.params.getrandom_out.buffer, recd);
to be on the safe side, I would do
/* never accept more bytes than we asked for */
recd = min(recd, num_bytes);
memcpy(dest, cmd.params.getrandom_out.buffer, recd);
so that the destination buffer can never be overwritten beyond its
boundaries by a TPM that just ends up sending you more bytes than you
asked for (firmware bug).
> +
> + dest += recd;
> + total += recd;
> + num_bytes -= recd;
> + } while (retries-- && total < max);
> +
> + return total ? total : -EIO;
> +}
> +
> +#define TPM2_GET_TPM_PT_IN_SIZE \
> + (sizeof(struct tpm_input_header) + \
> + sizeof(struct tpm2_get_tpm_pt_in))
> +
> +static const struct tpm_input_header tpm2_get_tpm_pt_header = {
> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
> + .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
> + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY)
> +};
> +
> +/**
> + * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property
> + * @chip: TPM chip to use.
> + * @property_id: property ID.
> + * @value: output variable.
> + * @desc: passed to tpm_transmit_cmd()
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value,
> + const char *desc)
> +{
> + struct tpm2_cmd cmd;
> + int rc;
> +
> + cmd.header.in = tpm2_get_tpm_pt_header;
> + cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
> + cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id);
> + cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
> +
> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
> + if (!rc)
> + *value = cmd.params.get_tpm_pt_out.value;
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt);
> +
> +/*
> + * tpm2_calc_ordinal_duration() - maximum duration for a command
> + * @chip: TPM chip to use.
> + * @ordinal: command code number.
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> +{
> + int index = TPM_UNDEFINED;
> + int duration = 0;
> +
> + if (ordinal >= TPM2_CC_FIRST && ordinal <= TPM2_CC_LAST)
> + index = tpm2_ordinal_duration[ordinal - TPM2_CC_FIRST];
> +
> + if (index != TPM_UNDEFINED)
> + duration = chip->vendor.duration[index];
> + if (duration <= 0)
> + return 2 * 60 * HZ;
> + else
> + return duration;
> +}
> +EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration);
> +
> +static const struct tpm_input_header tpm2_selftest_header = {
> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
> + .length = cpu_to_be32(sizeof(struct tpm_input_header) +
> + sizeof(struct tpm2_self_test_in)),
> + .ordinal = cpu_to_be32(TPM2_CC_SELF_TEST)
> +};
> +
> +#define TPM2_SELF_TEST_IN_SIZE \
> + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_self_test_in))
> +
> +/**
> + * tpm2_continue_selftest() - start a self test
> + * @chip: TPM chip to use
> + * @full: test all commands instead of testing only those that were not
> + * previously tested.
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +static int tpm2_start_selftest(struct tpm_chip *chip, bool full)
> +{
> + int rc;
> + struct tpm2_cmd cmd;
> +
> + cmd.header.in = tpm2_selftest_header;
> + cmd.params.selftest_in.full_test = full;
> +
> + rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE,
> + "continue selftest");
> +
> + return rc;
> +}
> +
> +/**
> + * tpm2_do_selftest() - run a full self test
> + * @chip: TPM chip to use
> + *
> + * During the self test TPM2 commands return with the error code RC_TESTING.
> + * Waiting is done by issuing PCR read until it executes successfully.
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +int tpm2_do_selftest(struct tpm_chip *chip)
> +{
> + int rc;
> + unsigned int loops;
> + unsigned int delay_msec = 100;
> + unsigned long duration;
> + struct tpm2_cmd cmd;
> + int i;
> +
> + duration = tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST);
> +
> + loops = jiffies_to_msecs(duration) / delay_msec;
> +
> + rc = tpm2_start_selftest(chip, true);
> + if (rc)
> + return rc;
> +
> + for (i = 0; i < loops; i++) {
> + /* Attempt to read a PCR value */
> + cmd.header.in = tpm2_pcrread_header;
> + cmd.params.pcrread_in.pcr_selects_cnt = cpu_to_be32(1);
> + cmd.params.pcrread_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
> + cmd.params.pcrread_in.pcr_select_size = TPM2_PCR_SELECT_MIN;
> + cmd.params.pcrread_in.pcr_select[0] = 0x01;
> + cmd.params.pcrread_in.pcr_select[1] = 0x00;
> + cmd.params.pcrread_in.pcr_select[2] = 0x00;
> +
> + rc = tpm_transmit_cmd(chip, (u8 *) &cmd, sizeof(cmd), NULL);
> + if (rc < 0)
> + break;
> +
> + rc = be32_to_cpu(cmd.header.out.return_code);
> + if (rc != TPM2_RC_TESTING)
> + break;
> +
> + msleep(delay_msec);
> + }
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm2_do_selftest);
> +
> +/**
> + * tpm2_gen_interrupt() - generate an interrupt
> + * @chip: TPM chip to use
> + *
> + * 0 is returned when the operation is successful. When a negative number is
> + * returned it remarks a POSIX error code. When a positive number is returned
> + * it remarks a TPM error.
> + */
> +
> +int tpm2_gen_interrupt(struct tpm_chip *chip)
> +{
> + u32 dummy;
> + int rc;
> +
> + rc = tpm2_get_tpm_pt(chip,
> + TPM2_CAP_TPM_PROPERTIES,
> + &dummy,
> + "attempting to generate an interrupt");
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm2_gen_interrupt);
Stefan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox