* [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-12 16:32 ` Wei Liu
2017-04-19 0:15 ` Stefano Stabellini
2017-04-03 9:44 ` [PATCH 02/10] xen/arm: vpl011: Add new virtual console hvm params " Bhupinder Thakur
` (9 subsequent siblings)
10 siblings, 2 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
Add emulation code to emulate read/write access to pl011 registers
and pl011 interrupts:
- Emulate DR read/write by reading and writing from/to the IN
and OUT ring buffers and raising an event to the backend when
there is data in the OUT ring buffer and injecting an interrupt
to the guest when there is data in the IN ring buffer
- Other registers are related to interrupt management and
essentially control when interrupts are delivered to the guest
The SBSA compliant pl011 uart is covered in Appendix B of
https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
xen/arch/arm/Kconfig | 5 +
xen/arch/arm/Makefile | 1 +
xen/arch/arm/vpl011.c | 339 +++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/domain.h | 5 +
xen/include/asm-arm/pl011-uart.h | 2 +
xen/include/public/arch-arm.h | 8 +
xen/include/xen/vpl011.h | 67 ++++++++
7 files changed, 427 insertions(+)
create mode 100644 xen/arch/arm/vpl011.c
create mode 100644 xen/include/xen/vpl011.h
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2e023d1..bfa86c7 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -45,6 +45,11 @@ config ACPI
config HAS_GICV3
bool
+config VPL011_CONSOLE
+ bool "Emulated pl011 console support"
+ default y
+ ---help---
+ Allows a guest to use pl011 UART as a console
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7afb8a3..a94bdab 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -49,6 +49,7 @@ obj-y += vm_event.o
obj-y += vtimer.o
obj-y += vpsci.o
obj-y += vuart.o
+obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o
#obj-bin-y += ....o
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
new file mode 100644
index 0000000..eeb1cbf
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,339 @@
+/*
+ * arch/arm/vpl011.c
+ *
+ * Virtual PL011 UART
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/sched.h>
+#include <xen/event.h>
+#include <public/io/console.h>
+#include <xen/vpl011.h>
+#include <asm-arm/pl011-uart.h>
+
+unsigned int vpl011_reg_mask[] = {0xff, 0xffff, 0xffffffff};
+
+static void vgic_inject_vpl011_spi(struct domain *d)
+{
+ struct vpl011_s *vpl011=&d->arch.vpl011;
+
+ if ( (vpl011->uartris & vpl011->uartimsc) )
+ vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
+}
+
+static void vpl011_read_data(struct domain *d, uint8_t *data)
+{
+ unsigned long flags;
+ struct vpl011_s *vpl011=&d->arch.vpl011;
+ struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
+
+ /*
+ * Initialize the data so that even if there is no data in ring buffer
+ * 0 is returned.
+ */
+ *data = 0;
+
+ VPL011_LOCK(d, flags);
+
+ /*
+ * It is expected that there will be data in the ring buffer when this
+ * function is called since the guest is expected to read the data register
+ * only if the TXFE flag is not set.
+ * If the guest still does read event when TXFE bit is set then 0 will
+ * be returned.
+ */
+ if ( !VPL011_IN_RING_EMPTY(intf) )
+ {
+ *data = intf->in[MASK_XENCONS_IDX(intf->in_cons++, intf->in)];
+ }
+
+ if ( VPL011_IN_RING_EMPTY(intf) )
+ {
+ vpl011->uartfr |= (RXFE);
+ vpl011->uartris &= ~(RXI);
+ }
+
+ vpl011->uartfr &= ~(RXFF);
+
+ VPL011_UNLOCK(d, flags);
+}
+
+static void vpl011_write_data(struct domain *d, uint8_t data)
+{
+ unsigned long flags;
+ struct vpl011_s *vpl011=&d->arch.vpl011;
+ struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
+
+ VPL011_LOCK(d, flags);
+
+ /*
+ * It is expected that the ring is not full when this function is called
+ * as the guest is expected to write to the data register only when the
+ * TXFF flag is not set.
+ * In case the guest does write even when the TXFF flag is set then the
+ * data will be silently dropped.
+ */
+ if ( !VPL011_OUT_RING_FULL(intf) )
+ {
+ intf->out[MASK_XENCONS_IDX(intf->out_prod++, intf->out)] = data;
+ smp_wmb();
+ }
+
+ if ( VPL011_OUT_RING_FULL(intf) )
+ {
+ vpl011->uartfr |= (TXFF);
+ vpl011->uartris &= ~(TXI);
+ }
+
+ vpl011->uartfr |= (BUSY);
+
+ vpl011->uartfr &= ~(TXFE);
+
+ VPL011_UNLOCK(d, flags);
+
+ /* raise an event to xenconsoled only if it is the first character in the buffer */
+ if ( VPL011_RING_DEPTH(intf, out) == 1 )
+ {
+ notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
+ }
+}
+
+static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv)
+{
+ uint8_t ch;
+ struct hsr_dabt dabt = info->dabt;
+ int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
+ struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
+
+ switch ( vpl011_reg )
+ {
+ case DR:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ vpl011_read_data(v->domain, &ch);
+ *r = ch;
+ break;
+
+ case RSR:
+ if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
+
+ /* It always returns 0 as there are no physical errors. */
+ *r = 0;
+ break;
+
+ case FR:
+ if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
+ *r = (vpl011->uartfr & vpl011_reg_mask[dabt.size]);
+ break;
+
+ case RIS:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ *r = (vpl011->uartris & vpl011_reg_mask[dabt.size]);
+ break;
+
+ case MIS:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ *r = (vpl011->uartris &
+ vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
+ break;
+
+ case IMSC:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ *r = (vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
+ break;
+
+ case ICR:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+
+ /* Only write is valid. */
+ return 0;
+
+ default:
+ gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
+ dabt.reg, vpl011_reg);
+ return 0;
+ }
+
+ return 1;
+
+bad_width:
+ gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
+ dabt.size, dabt.reg, vpl011_reg);
+ domain_crash_synchronous();
+ return 0;
+
+}
+
+static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv)
+{
+ uint8_t ch = ((struct uartdr_reg *)&r)->data;
+ struct hsr_dabt dabt = info->dabt;
+ int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
+ struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
+
+ switch ( vpl011_reg )
+ {
+ case DR:
+
+ if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
+ vpl011_write_data(v->domain, ch);
+ break;
+
+ case RSR: /* Nothing to clear. */
+ if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
+ break;
+
+ case FR:
+ goto write_ignore;
+ case RIS:
+ case MIS:
+ goto word_write_ignore;
+
+ case IMSC:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ vpl011->uartimsc = (r & vpl011_reg_mask[dabt.size]);
+ vgic_inject_vpl011_spi(v->domain);
+ break;
+
+ case ICR:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ vpl011->uartris &= ~(r & vpl011_reg_mask[dabt.size]);
+ vgic_inject_vpl011_spi(v->domain);
+ break;
+
+ default:
+ gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
+ dabt.reg, vpl011_reg);
+ return 0;
+ }
+
+ return 1;
+
+write_ignore:
+ if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
+ return 1;
+
+word_write_ignore:
+ if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
+ return 1;
+
+bad_width:
+ gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
+ dabt.size, dabt.reg, vpl011_reg);
+ domain_crash_synchronous();
+ return 0;
+
+}
+
+static const struct mmio_handler_ops vpl011_mmio_handler = {
+ .read = vpl011_mmio_read,
+ .write = vpl011_mmio_write,
+};
+
+int vpl011_map_guest_page(struct domain *d)
+{
+ struct vpl011_s *vpl011 = &d->arch.vpl011;
+
+ /* Map the guest PFN to Xen address space. */
+ return prepare_ring_for_helper(d,
+ d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_PFN],
+ &vpl011->ring_page,
+ &vpl011->ring_buf);
+}
+
+static void vpl011_data_avail(struct domain *d)
+{
+ unsigned long flags;
+ struct vpl011_s *vpl011 = &d->arch.vpl011;
+ struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
+
+ VPL011_LOCK(d, flags);
+
+ /* Update the uart rx state if the buffer is not empty. */
+ if ( !VPL011_IN_RING_EMPTY(intf) )
+ {
+ vpl011->uartfr &= ~(RXFE);
+ if ( VPL011_IN_RING_FULL(intf) )
+ vpl011->uartfr |= (RXFF);
+ vpl011->uartris |= (RXI);
+ }
+
+ /* Update the uart tx state if the buffer is not full. */
+ if ( !VPL011_OUT_RING_FULL(intf) )
+ {
+ vpl011->uartfr &= ~(TXFF);
+ vpl011->uartris |= (TXI);
+ if ( VPL011_OUT_RING_EMPTY(intf) )
+ {
+ vpl011->uartfr &= ~(BUSY);
+ vpl011->uartfr |= (TXFE);
+ }
+ }
+
+ VPL011_UNLOCK(d, flags);
+
+ vgic_inject_vpl011_spi(d);
+
+ if ( !VPL011_OUT_RING_EMPTY(intf) )
+ {
+ ASSERT( d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] != 0 );
+ notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
+ }
+}
+
+
+static void vpl011_notification(struct vcpu *v, unsigned int port)
+{
+ vpl011_data_avail(v->domain);
+}
+
+int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config)
+{
+ int rc;
+ struct vpl011_s *vpl011 = &d->arch.vpl011;
+
+ rc = alloc_unbound_xen_event_channel(d, 0, config->console_domid,
+ vpl011_notification);
+ if (rc < 0)
+ {
+ return rc;
+ }
+ d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] = rc;
+ rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
+ if ( !rc )
+ return rc;
+ register_mmio_handler(d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
+ spin_lock_init(&vpl011->lock);
+
+ vpl011->intialized = true;
+
+ return 0;
+}
+
+int domain_vpl011_deinit(struct domain *d)
+{
+ struct vpl011_s *vpl011 = &d->arch.vpl011;
+
+ if ( vpl011->intialized )
+ {
+ free_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
+ destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+ }
+
+ return 0;
+}
+
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2d6fbb1..a122504 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -11,6 +11,7 @@
#include <asm/gic.h>
#include <public/hvm/params.h>
#include <xen/serial.h>
+#include <xen/vpl011.h>
struct hvm_domain
{
@@ -131,6 +132,10 @@ struct arch_domain
struct {
uint8_t privileged_call_enabled : 1;
} monitor;
+
+#ifdef CONFIG_VPL011_CONSOLE
+ struct vpl011_s vpl011;
+#endif
} __cacheline_aligned;
struct arch_vcpu
diff --git a/xen/include/asm-arm/pl011-uart.h b/xen/include/asm-arm/pl011-uart.h
index 123f477..57e9ec7 100644
--- a/xen/include/asm-arm/pl011-uart.h
+++ b/xen/include/asm-arm/pl011-uart.h
@@ -49,6 +49,8 @@
/* FR bits */
#define TXFE (1<<7) /* TX FIFO empty */
#define RXFE (1<<4) /* RX FIFO empty */
+#define TXFF (1<<5) /* TX FIFO full */
+#define RXFF (1<<6) /* RX FIFO full */
#define BUSY (1<<3) /* Transmit is not complete */
/* LCR_H bits */
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index bd974fb..5f91207 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -322,6 +322,8 @@ struct xen_arch_domainconfig {
*
*/
uint32_t clock_frequency;
+
+ uint32_t console_domid;
};
#endif /* __XEN__ || __XEN_TOOLS__ */
@@ -410,6 +412,10 @@ typedef uint64_t xen_callback_t;
#define GUEST_ACPI_BASE 0x20000000ULL
#define GUEST_ACPI_SIZE 0x02000000ULL
+/* PL011 mappings */
+#define GUEST_PL011_BASE 0x22000000ULL
+#define GUEST_PL011_SIZE 0x00001000ULL
+
/*
* 16MB == 4096 pages reserved for guest to use as a region to map its
* grant table in.
@@ -444,6 +450,8 @@ typedef uint64_t xen_callback_t;
#define GUEST_TIMER_PHYS_NS_PPI 30
#define GUEST_EVTCHN_PPI 31
+#define GUEST_VPL011_SPI 32
+
/* PSCI functions */
#define PSCI_cpu_suspend 0
#define PSCI_cpu_off 1
diff --git a/xen/include/xen/vpl011.h b/xen/include/xen/vpl011.h
new file mode 100644
index 0000000..f9f2aba
--- /dev/null
+++ b/xen/include/xen/vpl011.h
@@ -0,0 +1,67 @@
+/*
+ * include/xen/vpl011.h
+ *
+ * Virtual PL011 UART
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _VPL011_H_
+
+#define _VPL011_H_
+
+/* helper macros */
+#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ## _cons))
+
+#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1)
+
+#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0)
+
+#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0)
+
+#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == VPL011_RING_MAX_DEPTH(intf, in))
+
+#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == VPL011_RING_MAX_DEPTH(intf,out))
+
+#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
+#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
+
+#define VALID_BW_SIZE(size) ( size == DABT_BYTE || size == DABT_HALF_WORD || size == DABT_WORD )
+#define VALID_W_SIZE(size) ( size == DABT_HALF_WORD || size == DABT_WORD )
+
+struct uartdr_reg {
+ uint8_t data;
+ uint8_t error_status:4;
+ uint8_t reserved1:4;
+ uint16_t reserved2;
+ uint32_t reserved3;
+};
+
+struct vpl011_s {
+ void *ring_buf;
+ struct page_info *ring_page;
+ uint32_t uartfr; /* flag register */
+ uint32_t uartcr; /* control register */
+ uint32_t uartimsc; /* interrupt mask register*/
+ uint32_t uarticr; /* interrupt clear register */
+ uint32_t uartris; /* raw interrupt status register */
+ uint32_t uartmis; /* masked interrupt register */
+ spinlock_t lock;
+ bool intialized; /* flag which tells whether vpl011 is initialized */
+};
+
+int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config);
+int domain_vpl011_deinit(struct domain *d);
+int vpl011_map_guest_page(struct domain *d);
+
+#endif
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-03 9:44 ` [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation " Bhupinder Thakur
@ 2017-04-12 16:32 ` Wei Liu
2017-04-19 0:15 ` Stefano Stabellini
1 sibling, 0 replies; 53+ messages in thread
From: Wei Liu @ 2017-04-12 16:32 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, Apr 03, 2017 at 03:14:24PM +0530, Bhupinder Thakur wrote:
> +
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/guest_access.h>
> +#include <xen/sched.h>
> +#include <xen/event.h>
> +#include <public/io/console.h>
> +#include <xen/vpl011.h>
> +#include <asm-arm/pl011-uart.h>
Normally I would place all xen headers first, then public headers, then
asm headers. And then please sort them alphabetically.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-03 9:44 ` [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation " Bhupinder Thakur
2017-04-12 16:32 ` Wei Liu
@ 2017-04-19 0:15 ` Stefano Stabellini
2017-04-19 7:28 ` Bhupinder Thakur
1 sibling, 1 reply; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 0:15 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> Add emulation code to emulate read/write access to pl011 registers
> and pl011 interrupts:
>
> - Emulate DR read/write by reading and writing from/to the IN
> and OUT ring buffers and raising an event to the backend when
> there is data in the OUT ring buffer and injecting an interrupt
> to the guest when there is data in the IN ring buffer
>
> - Other registers are related to interrupt management and
> essentially control when interrupts are delivered to the guest
>
> The SBSA compliant pl011 uart is covered in Appendix B of
> https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> xen/arch/arm/Kconfig | 5 +
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/vpl011.c | 339 +++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/domain.h | 5 +
> xen/include/asm-arm/pl011-uart.h | 2 +
> xen/include/public/arch-arm.h | 8 +
> xen/include/xen/vpl011.h | 67 ++++++++
> 7 files changed, 427 insertions(+)
> create mode 100644 xen/arch/arm/vpl011.c
> create mode 100644 xen/include/xen/vpl011.h
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 2e023d1..bfa86c7 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -45,6 +45,11 @@ config ACPI
> config HAS_GICV3
> bool
>
> +config VPL011_CONSOLE
> + bool "Emulated pl011 console support"
> + default y
> + ---help---
> + Allows a guest to use pl011 UART as a console
> endmenu
>
> menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7afb8a3..a94bdab 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -49,6 +49,7 @@ obj-y += vm_event.o
> obj-y += vtimer.o
> obj-y += vpsci.o
> obj-y += vuart.o
> +obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o
>
> #obj-bin-y += ....o
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> new file mode 100644
> index 0000000..eeb1cbf
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
> @@ -0,0 +1,339 @@
> +/*
> + * arch/arm/vpl011.c
> + *
> + * Virtual PL011 UART
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/guest_access.h>
> +#include <xen/sched.h>
> +#include <xen/event.h>
> +#include <public/io/console.h>
> +#include <xen/vpl011.h>
> +#include <asm-arm/pl011-uart.h>
> +
> +unsigned int vpl011_reg_mask[] = {0xff, 0xffff, 0xffffffff};
> +
> +static void vgic_inject_vpl011_spi(struct domain *d)
> +{
> + struct vpl011_s *vpl011=&d->arch.vpl011;
> +
> + if ( (vpl011->uartris & vpl011->uartimsc) )
> + vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> +}
> +
> +static void vpl011_read_data(struct domain *d, uint8_t *data)
> +{
> + unsigned long flags;
> + struct vpl011_s *vpl011=&d->arch.vpl011;
The code style is:
vpl011 = &d->arch.vpl011
> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
Same here. Please fix it everywhere.
> + /*
> + * Initialize the data so that even if there is no data in ring buffer
> + * 0 is returned.
> + */
> + *data = 0;
> +
> + VPL011_LOCK(d, flags);
> +
> + /*
> + * It is expected that there will be data in the ring buffer when this
> + * function is called since the guest is expected to read the data register
> + * only if the TXFE flag is not set.
> + * If the guest still does read event when TXFE bit is set then 0 will
> + * be returned.
> + */
> + if ( !VPL011_IN_RING_EMPTY(intf) )
> + {
> + *data = intf->in[MASK_XENCONS_IDX(intf->in_cons++, intf->in)];
For correctness, it is important to maintain the right ordering and use
barriers:
in_cons = intf->in_cons;
*data = intf->in[MASK_XENCONS_IDX(in_cons, intf->in)];
smb_mb();
intf->in_cons = in_cons + 1;
> + }
> +
> + if ( VPL011_IN_RING_EMPTY(intf) )
> + {
> + vpl011->uartfr |= (RXFE);
> + vpl011->uartris &= ~(RXI);
> + }
> +
> + vpl011->uartfr &= ~(RXFF);
> +
> + VPL011_UNLOCK(d, flags);
I think we need to notify xenconsoled here, in case the ring was
previosuly full? Now that we have read some data, xenconsoled can go
back and write more.
> +}
> +
> +static void vpl011_write_data(struct domain *d, uint8_t data)
> +{
> + unsigned long flags;
> + struct vpl011_s *vpl011=&d->arch.vpl011;
> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
> +
> + VPL011_LOCK(d, flags);
> +
> + /*
> + * It is expected that the ring is not full when this function is called
> + * as the guest is expected to write to the data register only when the
> + * TXFF flag is not set.
> + * In case the guest does write even when the TXFF flag is set then the
> + * data will be silently dropped.
> + */
> + if ( !VPL011_OUT_RING_FULL(intf) )
> + {
> + intf->out[MASK_XENCONS_IDX(intf->out_prod++, intf->out)] = data;
> + smp_wmb();
Similarly, the order should be:
out_prod = intf->out_prod;
intf->out[MASK_XENCONS_IDX(out_prod, intf->out)] = data;
smb_wmb();
intf->out_prod = out_prod + 1;
> + }
> +
> + if ( VPL011_OUT_RING_FULL(intf) )
> + {
> + vpl011->uartfr |= (TXFF);
> + vpl011->uartris &= ~(TXI);
> + }
> +
> + vpl011->uartfr |= (BUSY);
> +
> + vpl011->uartfr &= ~(TXFE);
> +
> + VPL011_UNLOCK(d, flags);
> +
> + /* raise an event to xenconsoled only if it is the first character in the buffer */
> + if ( VPL011_RING_DEPTH(intf, out) == 1 )
> + {
> + notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
> + }
Why only in case is the first character? xenconsoled will miss updates
if we don't send a notification, right?
> +}
> +
> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv)
> +{
> + uint8_t ch;
> + struct hsr_dabt dabt = info->dabt;
> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> +
> + switch ( vpl011_reg )
> + {
> + case DR:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + vpl011_read_data(v->domain, &ch);
> + *r = ch;
> + break;
> +
> + case RSR:
> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> +
> + /* It always returns 0 as there are no physical errors. */
> + *r = 0;
> + break;
> +
> + case FR:
> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> + *r = (vpl011->uartfr & vpl011_reg_mask[dabt.size]);
> + break;
> +
> + case RIS:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + *r = (vpl011->uartris & vpl011_reg_mask[dabt.size]);
> + break;
> +
> + case MIS:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + *r = (vpl011->uartris &
> + vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
> + break;
> +
> + case IMSC:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + *r = (vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
> + break;
> +
> + case ICR:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> +
> + /* Only write is valid. */
> + return 0;
> +
> + default:
> + gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
> + dabt.reg, vpl011_reg);
> + return 0;
> + }
> +
> + return 1;
> +
> +bad_width:
> + gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
> + dabt.size, dabt.reg, vpl011_reg);
> + domain_crash_synchronous();
> + return 0;
> +
> +}
> +
> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv)
> +{
> + uint8_t ch = ((struct uartdr_reg *)&r)->data;
> + struct hsr_dabt dabt = info->dabt;
> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> +
> + switch ( vpl011_reg )
> + {
> + case DR:
> +
> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> + vpl011_write_data(v->domain, ch);
> + break;
> +
> + case RSR: /* Nothing to clear. */
> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> + break;
> +
> + case FR:
> + goto write_ignore;
> + case RIS:
> + case MIS:
> + goto word_write_ignore;
> +
> + case IMSC:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + vpl011->uartimsc = (r & vpl011_reg_mask[dabt.size]);
> + vgic_inject_vpl011_spi(v->domain);
> + break;
> +
> + case ICR:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + vpl011->uartris &= ~(r & vpl011_reg_mask[dabt.size]);
> + vgic_inject_vpl011_spi(v->domain);
> + break;
> +
> + default:
> + gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
> + dabt.reg, vpl011_reg);
> + return 0;
> + }
> +
> + return 1;
> +
> +write_ignore:
> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> + return 1;
> +
> +word_write_ignore:
> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> + return 1;
> +
> +bad_width:
> + gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
> + dabt.size, dabt.reg, vpl011_reg);
> + domain_crash_synchronous();
> + return 0;
> +
> +}
> +
> +static const struct mmio_handler_ops vpl011_mmio_handler = {
> + .read = vpl011_mmio_read,
> + .write = vpl011_mmio_write,
> +};
> +
> +int vpl011_map_guest_page(struct domain *d)
> +{
> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> + /* Map the guest PFN to Xen address space. */
> + return prepare_ring_for_helper(d,
> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_PFN],
> + &vpl011->ring_page,
> + &vpl011->ring_buf);
> +}
> +
> +static void vpl011_data_avail(struct domain *d)
> +{
> + unsigned long flags;
> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
> +
> + VPL011_LOCK(d, flags);
> +
> + /* Update the uart rx state if the buffer is not empty. */
> + if ( !VPL011_IN_RING_EMPTY(intf) )
> + {
> + vpl011->uartfr &= ~(RXFE);
> + if ( VPL011_IN_RING_FULL(intf) )
> + vpl011->uartfr |= (RXFF);
> + vpl011->uartris |= (RXI);
> + }
> +
> + /* Update the uart tx state if the buffer is not full. */
> + if ( !VPL011_OUT_RING_FULL(intf) )
> + {
> + vpl011->uartfr &= ~(TXFF);
> + vpl011->uartris |= (TXI);
> + if ( VPL011_OUT_RING_EMPTY(intf) )
> + {
> + vpl011->uartfr &= ~(BUSY);
> + vpl011->uartfr |= (TXFE);
It is not a good idea to check the intf values twice: xenconsoled might
be modifying them, so we can get into the situation where intf->out*
change in between the two checks. Please read out_cons and out_prod only
once in this function.
> + }
> + }
> +
> + VPL011_UNLOCK(d, flags);
> +
> + vgic_inject_vpl011_spi(d);
> +
> + if ( !VPL011_OUT_RING_EMPTY(intf) )
> + {
> + ASSERT( d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] != 0 );
> + notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
Why are we sending notifications at this point? The out buffer hasn't
changed since xenconsoled sent the evtchn to Xen, so xenconsoled already
knows at this point that the out ring is not empty.
> + }
> +}
> +
> +
> +static void vpl011_notification(struct vcpu *v, unsigned int port)
> +{
> + vpl011_data_avail(v->domain);
> +}
> +
> +int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config)
> +{
> + int rc;
> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> + rc = alloc_unbound_xen_event_channel(d, 0, config->console_domid,
> + vpl011_notification);
> + if (rc < 0)
> + {
> + return rc;
> + }
> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] = rc;
> + rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> + if ( !rc )
> + return rc;
We should free the allocated evtchn in case of error here
> + register_mmio_handler(d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> + spin_lock_init(&vpl011->lock);
> +
> + vpl011->intialized = true;
> +
> + return 0;
> +}
> +
> +int domain_vpl011_deinit(struct domain *d)
> +{
> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> + if ( vpl011->intialized )
> + {
> + free_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
> + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> + }
> +
> + return 0;
> +}
> +
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2d6fbb1..a122504 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -11,6 +11,7 @@
> #include <asm/gic.h>
> #include <public/hvm/params.h>
> #include <xen/serial.h>
> +#include <xen/vpl011.h>
>
> struct hvm_domain
> {
> @@ -131,6 +132,10 @@ struct arch_domain
> struct {
> uint8_t privileged_call_enabled : 1;
> } monitor;
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> + struct vpl011_s vpl011;
> +#endif
> } __cacheline_aligned;
>
> struct arch_vcpu
> diff --git a/xen/include/asm-arm/pl011-uart.h b/xen/include/asm-arm/pl011-uart.h
> index 123f477..57e9ec7 100644
> --- a/xen/include/asm-arm/pl011-uart.h
> +++ b/xen/include/asm-arm/pl011-uart.h
> @@ -49,6 +49,8 @@
> /* FR bits */
> #define TXFE (1<<7) /* TX FIFO empty */
> #define RXFE (1<<4) /* RX FIFO empty */
> +#define TXFF (1<<5) /* TX FIFO full */
> +#define RXFF (1<<6) /* RX FIFO full */
> #define BUSY (1<<3) /* Transmit is not complete */
>
> /* LCR_H bits */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..5f91207 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -322,6 +322,8 @@ struct xen_arch_domainconfig {
> *
> */
> uint32_t clock_frequency;
> +
> + uint32_t console_domid;
> };
> #endif /* __XEN__ || __XEN_TOOLS__ */
>
> @@ -410,6 +412,10 @@ typedef uint64_t xen_callback_t;
> #define GUEST_ACPI_BASE 0x20000000ULL
> #define GUEST_ACPI_SIZE 0x02000000ULL
>
> +/* PL011 mappings */
> +#define GUEST_PL011_BASE 0x22000000ULL
> +#define GUEST_PL011_SIZE 0x00001000ULL
> +
> /*
> * 16MB == 4096 pages reserved for guest to use as a region to map its
> * grant table in.
> @@ -444,6 +450,8 @@ typedef uint64_t xen_callback_t;
> #define GUEST_TIMER_PHYS_NS_PPI 30
> #define GUEST_EVTCHN_PPI 31
>
> +#define GUEST_VPL011_SPI 32
> +
> /* PSCI functions */
> #define PSCI_cpu_suspend 0
> #define PSCI_cpu_off 1
> diff --git a/xen/include/xen/vpl011.h b/xen/include/xen/vpl011.h
> new file mode 100644
> index 0000000..f9f2aba
> --- /dev/null
> +++ b/xen/include/xen/vpl011.h
> @@ -0,0 +1,67 @@
> +/*
> + * include/xen/vpl011.h
> + *
> + * Virtual PL011 UART
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef _VPL011_H_
> +
> +#define _VPL011_H_
> +
> +/* helper macros */
> +#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ## _cons))
> +
> +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1)
> +
> +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0)
> +
> +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0)
> +
> +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == VPL011_RING_MAX_DEPTH(intf, in))
> +
> +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == VPL011_RING_MAX_DEPTH(intf,out))
> +
> +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
> +#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
> +
> +#define VALID_BW_SIZE(size) ( size == DABT_BYTE || size == DABT_HALF_WORD || size == DABT_WORD )
> +#define VALID_W_SIZE(size) ( size == DABT_HALF_WORD || size == DABT_WORD )
> +
> +struct uartdr_reg {
> + uint8_t data;
> + uint8_t error_status:4;
> + uint8_t reserved1:4;
> + uint16_t reserved2;
> + uint32_t reserved3;
> +};
> +
> +struct vpl011_s {
> + void *ring_buf;
> + struct page_info *ring_page;
> + uint32_t uartfr; /* flag register */
> + uint32_t uartcr; /* control register */
> + uint32_t uartimsc; /* interrupt mask register*/
> + uint32_t uarticr; /* interrupt clear register */
> + uint32_t uartris; /* raw interrupt status register */
> + uint32_t uartmis; /* masked interrupt register */
> + spinlock_t lock;
> + bool intialized; /* flag which tells whether vpl011 is initialized */
> +};
> +
> +int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config);
> +int domain_vpl011_deinit(struct domain *d);
> +int vpl011_map_guest_page(struct domain *d);
> +
> +#endif
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-19 0:15 ` Stefano Stabellini
@ 2017-04-19 7:28 ` Bhupinder Thakur
2017-04-19 8:36 ` Julien Grall
2017-04-19 18:40 ` Stefano Stabellini
0 siblings, 2 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-19 7:28 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Ian Jackson, Wei Liu
Hi Stefano,
Thanks for your comments.
On 19 April 2017 at 05:45, Stefano Stabellini <sstabellini@kernel.org> wrote:
>> +static void vpl011_read_data(struct domain *d, uint8_t *data)
>> +{
>> + unsigned long flags;
>> + struct vpl011_s *vpl011=&d->arch.vpl011;
>
> The code style is:
>
> vpl011 = &d->arch.vpl011
>
>
>> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
>
> Same here. Please fix it everywhere.
>
I will fix the coding style here.
>
>> + /*
>> + * Initialize the data so that even if there is no data in ring buffer
>> + * 0 is returned.
>> + */
>> + *data = 0;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + /*
>> + * It is expected that there will be data in the ring buffer when this
>> + * function is called since the guest is expected to read the data register
>> + * only if the TXFE flag is not set.
>> + * If the guest still does read event when TXFE bit is set then 0 will
>> + * be returned.
>> + */
>> + if ( !VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + *data = intf->in[MASK_XENCONS_IDX(intf->in_cons++, intf->in)];
>
> For correctness, it is important to maintain the right ordering and use
> barriers:
>
> in_cons = intf->in_cons;
> *data = intf->in[MASK_XENCONS_IDX(in_cons, intf->in)];
> smb_mb();
> intf->in_cons = in_cons + 1;
>
>
>> + }
>> +
>> + if ( VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + vpl011->uartfr |= (RXFE);
>> + vpl011->uartris &= ~(RXI);
>> + }
>> +
>> + vpl011->uartfr &= ~(RXFF);
>> +
>> + VPL011_UNLOCK(d, flags);
>
> I think we need to notify xenconsoled here, in case the ring was
> previosuly full? Now that we have read some data, xenconsoled can go
> back and write more.
>
Yes this is a valid point. I will send an event here.
>
>> +}
>> +
>> +static void vpl011_write_data(struct domain *d, uint8_t data)
>> +{
>> + unsigned long flags;
>> + struct vpl011_s *vpl011=&d->arch.vpl011;
>> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + /*
>> + * It is expected that the ring is not full when this function is called
>> + * as the guest is expected to write to the data register only when the
>> + * TXFF flag is not set.
>> + * In case the guest does write even when the TXFF flag is set then the
>> + * data will be silently dropped.
>> + */
>> + if ( !VPL011_OUT_RING_FULL(intf) )
>> + {
>> + intf->out[MASK_XENCONS_IDX(intf->out_prod++, intf->out)] = data;
>> + smp_wmb();
>
> Similarly, the order should be:
>
> out_prod = intf->out_prod;
> intf->out[MASK_XENCONS_IDX(out_prod, intf->out)] = data;
> smb_wmb();
> intf->out_prod = out_prod + 1;
>
I will use the suggested order.
>
>> + }
>> +
>> + if ( VPL011_OUT_RING_FULL(intf) )
>> + {
>> + vpl011->uartfr |= (TXFF);
>> + vpl011->uartris &= ~(TXI);
>> + }
>> +
>> + vpl011->uartfr |= (BUSY);
>> +
>> + vpl011->uartfr &= ~(TXFE);
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + /* raise an event to xenconsoled only if it is the first character in the buffer */
>> + if ( VPL011_RING_DEPTH(intf, out) == 1 )
>> + {
>> + notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
>> + }
>
> Why only in case is the first character? xenconsoled will miss updates
> if we don't send a notification, right?
>
Whenever xenconsoled sends an event, the vpl011_data_avail function
will make sure that if there is data in the OUT ring buffer then it
again
sends an event to xenconsoled. Also it avoids the need of sending an
event for every character written in the OUT ring buffer.
>
>> +}
>> +
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv)
>> +{
>> + uint8_t ch;
>> + struct hsr_dabt dabt = info->dabt;
>> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> + switch ( vpl011_reg )
>> + {
>> + case DR:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + vpl011_read_data(v->domain, &ch);
>> + *r = ch;
>> + break;
>> +
>> + case RSR:
>> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> +
>> + /* It always returns 0 as there are no physical errors. */
>> + *r = 0;
>> + break;
>> +
>> + case FR:
>> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> + *r = (vpl011->uartfr & vpl011_reg_mask[dabt.size]);
>> + break;
>> +
>> + case RIS:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + *r = (vpl011->uartris & vpl011_reg_mask[dabt.size]);
>> + break;
>> +
>> + case MIS:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + *r = (vpl011->uartris &
>> + vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
>> + break;
>> +
>> + case IMSC:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + *r = (vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
>> + break;
>> +
>> + case ICR:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> +
>> + /* Only write is valid. */
>> + return 0;
>> +
>> + default:
>> + gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
>> + dabt.reg, vpl011_reg);
>> + return 0;
>> + }
>> +
>> + return 1;
>> +
>> +bad_width:
>> + gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
>> + dabt.size, dabt.reg, vpl011_reg);
>> + domain_crash_synchronous();
>> + return 0;
>> +
>> +}
>> +
>> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv)
>> +{
>> + uint8_t ch = ((struct uartdr_reg *)&r)->data;
>> + struct hsr_dabt dabt = info->dabt;
>> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> + switch ( vpl011_reg )
>> + {
>> + case DR:
>> +
>> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> + vpl011_write_data(v->domain, ch);
>> + break;
>> +
>> + case RSR: /* Nothing to clear. */
>> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> + break;
>> +
>> + case FR:
>> + goto write_ignore;
>> + case RIS:
>> + case MIS:
>> + goto word_write_ignore;
>> +
>> + case IMSC:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + vpl011->uartimsc = (r & vpl011_reg_mask[dabt.size]);
>> + vgic_inject_vpl011_spi(v->domain);
>> + break;
>> +
>> + case ICR:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + vpl011->uartris &= ~(r & vpl011_reg_mask[dabt.size]);
>> + vgic_inject_vpl011_spi(v->domain);
>> + break;
>> +
>> + default:
>> + gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
>> + dabt.reg, vpl011_reg);
>> + return 0;
>> + }
>> +
>> + return 1;
>> +
>> +write_ignore:
>> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> + return 1;
>> +
>> +word_write_ignore:
>> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> + return 1;
>> +
>> +bad_width:
>> + gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
>> + dabt.size, dabt.reg, vpl011_reg);
>> + domain_crash_synchronous();
>> + return 0;
>> +
>> +}
>> +
>> +static const struct mmio_handler_ops vpl011_mmio_handler = {
>> + .read = vpl011_mmio_read,
>> + .write = vpl011_mmio_write,
>> +};
>> +
>> +int vpl011_map_guest_page(struct domain *d)
>> +{
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> + /* Map the guest PFN to Xen address space. */
>> + return prepare_ring_for_helper(d,
>> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_PFN],
>> + &vpl011->ring_page,
>> + &vpl011->ring_buf);
>> +}
>> +
>> +static void vpl011_data_avail(struct domain *d)
>> +{
>> + unsigned long flags;
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + /* Update the uart rx state if the buffer is not empty. */
>> + if ( !VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + vpl011->uartfr &= ~(RXFE);
>> + if ( VPL011_IN_RING_FULL(intf) )
>> + vpl011->uartfr |= (RXFF);
>> + vpl011->uartris |= (RXI);
>> + }
>> +
>> + /* Update the uart tx state if the buffer is not full. */
>> + if ( !VPL011_OUT_RING_FULL(intf) )
>> + {
>> + vpl011->uartfr &= ~(TXFF);
>> + vpl011->uartris |= (TXI);
>> + if ( VPL011_OUT_RING_EMPTY(intf) )
>> + {
>> + vpl011->uartfr &= ~(BUSY);
>> + vpl011->uartfr |= (TXFE);
>
> It is not a good idea to check the intf values twice: xenconsoled might
> be modifying them, so we can get into the situation where intf->out*
> change in between the two checks. Please read out_cons and out_prod only
> once in this function.
Since this code is under LOCK, the IN and OUT ring buffers will not be
updated by the guest. Specifically, the following transitions are
ruled out:
IN ring buffer
non-empty ----> empty (as the reader is blocked due to lock)
OUT ring buffer
not-full ----> full (as writer is blocked due to lock).
So the code inside the IF block remains valid even if the buffer state changes.
For the IN ring buffer it can go from non-empty to full. Similarly for
OUT ring buffer it can go from FULL to empty.
Also checking the latest buffer index (instead of checking buffer
index read as local variables) allows to update the pl011 state at the
earliest.
>
>
>> + }
>> + }
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + vgic_inject_vpl011_spi(d);
>> +
>> + if ( !VPL011_OUT_RING_EMPTY(intf) )
>> + {
>> + ASSERT( d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] != 0 );
>> + notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
>
> Why are we sending notifications at this point? The out buffer hasn't
> changed since xenconsoled sent the evtchn to Xen, so xenconsoled already
> knows at this point that the out ring is not empty.
The OUT buffer may be empty when xenconsoled sends the event. By the
time the event is handled, the guest may write more data to OUT
buffer. This condition makes sure that if the OUT ring buffer is not
empty then it sends an event to xenconsoled.
>
>
>> + }
>> +}
>> +
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> + vpl011_data_avail(v->domain);
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config)
>> +{
>> + int rc;
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> + rc = alloc_unbound_xen_event_channel(d, 0, config->console_domid,
>> + vpl011_notification);
>> + if (rc < 0)
>> + {
>> + return rc;
>> + }
>> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] = rc;
>> + rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>> + if ( !rc )
>> + return rc;
>
> We should free the allocated evtchn in case of error here
I will free the event channel.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-19 7:28 ` Bhupinder Thakur
@ 2017-04-19 8:36 ` Julien Grall
2017-04-19 18:40 ` Stefano Stabellini
1 sibling, 0 replies; 53+ messages in thread
From: Julien Grall @ 2017-04-19 8:36 UTC (permalink / raw)
To: Bhupinder Thakur, Stefano Stabellini; +Cc: xen-devel, Ian Jackson, Wei Liu
Hi Bhupinder,
On 04/19/2017 08:28 AM, Bhupinder Thakur wrote:
> On 19 April 2017 at 05:45, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> + }
>>> +}
>>> +
>>> +
>>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>>> +{
>>> + vpl011_data_avail(v->domain);
>>> +}
>>> +
>>> +int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config)
>>> +{
>>> + int rc;
>>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>>> +
>>> + rc = alloc_unbound_xen_event_channel(d, 0, config->console_domid,
>>> + vpl011_notification);
>>> + if (rc < 0)
>>> + {
>>> + return rc;
>>> + }
>>> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] = rc;
>>> + rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>>> + if ( !rc )
>>> + return rc;
>>
>> We should free the allocated evtchn in case of error here
> I will free the event channel.
You will also want to reset d->arch.hvm_domain.params[...] to avoid the
the other end reading garbage.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-19 7:28 ` Bhupinder Thakur
2017-04-19 8:36 ` Julien Grall
@ 2017-04-19 18:40 ` Stefano Stabellini
2017-04-25 7:31 ` Bhupinder Thakur
1 sibling, 1 reply; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 18:40 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Wed, 19 Apr 2017, Bhupinder Thakur wrote:
> Hi Stefano,
>
> Thanks for your comments.
>
> On 19 April 2017 at 05:45, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >> +static void vpl011_read_data(struct domain *d, uint8_t *data)
> >> +{
> >> + unsigned long flags;
> >> + struct vpl011_s *vpl011=&d->arch.vpl011;
> >
> > The code style is:
> >
> > vpl011 = &d->arch.vpl011
> >
> >
> >> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
> >
> > Same here. Please fix it everywhere.
> >
> I will fix the coding style here.
>
> >
> >> + /*
> >> + * Initialize the data so that even if there is no data in ring buffer
> >> + * 0 is returned.
> >> + */
> >> + *data = 0;
> >> +
> >> + VPL011_LOCK(d, flags);
> >> +
> >> + /*
> >> + * It is expected that there will be data in the ring buffer when this
> >> + * function is called since the guest is expected to read the data register
> >> + * only if the TXFE flag is not set.
> >> + * If the guest still does read event when TXFE bit is set then 0 will
> >> + * be returned.
> >> + */
> >> + if ( !VPL011_IN_RING_EMPTY(intf) )
> >> + {
> >> + *data = intf->in[MASK_XENCONS_IDX(intf->in_cons++, intf->in)];
> >
> > For correctness, it is important to maintain the right ordering and use
> > barriers:
> >
> > in_cons = intf->in_cons;
> > *data = intf->in[MASK_XENCONS_IDX(in_cons, intf->in)];
> > smb_mb();
> > intf->in_cons = in_cons + 1;
> >
> >
> >> + }
> >> +
> >> + if ( VPL011_IN_RING_EMPTY(intf) )
> >> + {
> >> + vpl011->uartfr |= (RXFE);
> >> + vpl011->uartris &= ~(RXI);
> >> + }
> >> +
> >> + vpl011->uartfr &= ~(RXFF);
> >> +
> >> + VPL011_UNLOCK(d, flags);
> >
> > I think we need to notify xenconsoled here, in case the ring was
> > previosuly full? Now that we have read some data, xenconsoled can go
> > back and write more.
> >
> Yes this is a valid point. I will send an event here.
>
> >
> >> +}
> >> +
> >> +static void vpl011_write_data(struct domain *d, uint8_t data)
> >> +{
> >> + unsigned long flags;
> >> + struct vpl011_s *vpl011=&d->arch.vpl011;
> >> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
> >> +
> >> + VPL011_LOCK(d, flags);
> >> +
> >> + /*
> >> + * It is expected that the ring is not full when this function is called
> >> + * as the guest is expected to write to the data register only when the
> >> + * TXFF flag is not set.
> >> + * In case the guest does write even when the TXFF flag is set then the
> >> + * data will be silently dropped.
> >> + */
> >> + if ( !VPL011_OUT_RING_FULL(intf) )
> >> + {
> >> + intf->out[MASK_XENCONS_IDX(intf->out_prod++, intf->out)] = data;
> >> + smp_wmb();
> >
> > Similarly, the order should be:
> >
> > out_prod = intf->out_prod;
> > intf->out[MASK_XENCONS_IDX(out_prod, intf->out)] = data;
> > smb_wmb();
> > intf->out_prod = out_prod + 1;
> >
> I will use the suggested order.
> >
> >> + }
> >> +
> >> + if ( VPL011_OUT_RING_FULL(intf) )
> >> + {
> >> + vpl011->uartfr |= (TXFF);
> >> + vpl011->uartris &= ~(TXI);
> >> + }
> >> +
> >> + vpl011->uartfr |= (BUSY);
> >> +
> >> + vpl011->uartfr &= ~(TXFE);
> >> +
> >> + VPL011_UNLOCK(d, flags);
> >> +
> >> + /* raise an event to xenconsoled only if it is the first character in the buffer */
> >> + if ( VPL011_RING_DEPTH(intf, out) == 1 )
> >> + {
> >> + notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
> >> + }
> >
> > Why only in case is the first character? xenconsoled will miss updates
> > if we don't send a notification, right?
> >
> Whenever xenconsoled sends an event, the vpl011_data_avail function
> will make sure that if there is data in the OUT ring buffer then it
> again
> sends an event to xenconsoled. Also it avoids the need of sending an
> event for every character written in the OUT ring buffer.
We don't have a formal protocol spec for PV console, but if we had, it
would say that the frontend (Xen in this case) should send notifications
every time there is new data to write. Thus, once per character in this
case.
I would start with a dumb implementation like that, simply notify the
other end every time. Then, in a separate patch, we can consider
introducing optimizations, which need to be well explained.
Regarding the optimization you introduced in this patch, delaying write
notifications until we receive a notification from xenconsoled, how many
notifications from xen to xenconsoled does it actually save? xenconsoled
is going to send a notification for every read: we might end up sending
the same number of notifications, only delayed.
> >> +}
> >> +
> >> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv)
> >> +{
> >> + uint8_t ch;
> >> + struct hsr_dabt dabt = info->dabt;
> >> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
> >> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> >> +
> >> + switch ( vpl011_reg )
> >> + {
> >> + case DR:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + vpl011_read_data(v->domain, &ch);
> >> + *r = ch;
> >> + break;
> >> +
> >> + case RSR:
> >> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> >> +
> >> + /* It always returns 0 as there are no physical errors. */
> >> + *r = 0;
> >> + break;
> >> +
> >> + case FR:
> >> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> >> + *r = (vpl011->uartfr & vpl011_reg_mask[dabt.size]);
> >> + break;
> >> +
> >> + case RIS:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + *r = (vpl011->uartris & vpl011_reg_mask[dabt.size]);
> >> + break;
> >> +
> >> + case MIS:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + *r = (vpl011->uartris &
> >> + vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
> >> + break;
> >> +
> >> + case IMSC:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + *r = (vpl011->uartimsc & vpl011_reg_mask[dabt.size]);
> >> + break;
> >> +
> >> + case ICR:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> +
> >> + /* Only write is valid. */
> >> + return 0;
> >> +
> >> + default:
> >> + gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
> >> + dabt.reg, vpl011_reg);
> >> + return 0;
> >> + }
> >> +
> >> + return 1;
> >> +
> >> +bad_width:
> >> + gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
> >> + dabt.size, dabt.reg, vpl011_reg);
> >> + domain_crash_synchronous();
> >> + return 0;
> >> +
> >> +}
> >> +
> >> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv)
> >> +{
> >> + uint8_t ch = ((struct uartdr_reg *)&r)->data;
> >> + struct hsr_dabt dabt = info->dabt;
> >> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
> >> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> >> +
> >> + switch ( vpl011_reg )
> >> + {
> >> + case DR:
> >> +
> >> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> >> + vpl011_write_data(v->domain, ch);
> >> + break;
> >> +
> >> + case RSR: /* Nothing to clear. */
> >> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> >> + break;
> >> +
> >> + case FR:
> >> + goto write_ignore;
> >> + case RIS:
> >> + case MIS:
> >> + goto word_write_ignore;
> >> +
> >> + case IMSC:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + vpl011->uartimsc = (r & vpl011_reg_mask[dabt.size]);
> >> + vgic_inject_vpl011_spi(v->domain);
> >> + break;
> >> +
> >> + case ICR:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + vpl011->uartris &= ~(r & vpl011_reg_mask[dabt.size]);
> >> + vgic_inject_vpl011_spi(v->domain);
> >> + break;
> >> +
> >> + default:
> >> + gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
> >> + dabt.reg, vpl011_reg);
> >> + return 0;
> >> + }
> >> +
> >> + return 1;
> >> +
> >> +write_ignore:
> >> + if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
> >> + return 1;
> >> +
> >> +word_write_ignore:
> >> + if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
> >> + return 1;
> >> +
> >> +bad_width:
> >> + gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
> >> + dabt.size, dabt.reg, vpl011_reg);
> >> + domain_crash_synchronous();
> >> + return 0;
> >> +
> >> +}
> >> +
> >> +static const struct mmio_handler_ops vpl011_mmio_handler = {
> >> + .read = vpl011_mmio_read,
> >> + .write = vpl011_mmio_write,
> >> +};
> >> +
> >> +int vpl011_map_guest_page(struct domain *d)
> >> +{
> >> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> >> +
> >> + /* Map the guest PFN to Xen address space. */
> >> + return prepare_ring_for_helper(d,
> >> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_PFN],
> >> + &vpl011->ring_page,
> >> + &vpl011->ring_buf);
> >> +}
> >> +
> >> +static void vpl011_data_avail(struct domain *d)
> >> +{
> >> + unsigned long flags;
> >> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> >> + struct xencons_interface *intf=(struct xencons_interface *)vpl011->ring_buf;
> >> +
> >> + VPL011_LOCK(d, flags);
> >> +
> >> + /* Update the uart rx state if the buffer is not empty. */
> >> + if ( !VPL011_IN_RING_EMPTY(intf) )
> >> + {
> >> + vpl011->uartfr &= ~(RXFE);
> >> + if ( VPL011_IN_RING_FULL(intf) )
> >> + vpl011->uartfr |= (RXFF);
> >> + vpl011->uartris |= (RXI);
> >> + }
> >> +
> >> + /* Update the uart tx state if the buffer is not full. */
> >> + if ( !VPL011_OUT_RING_FULL(intf) )
> >> + {
> >> + vpl011->uartfr &= ~(TXFF);
> >> + vpl011->uartris |= (TXI);
> >> + if ( VPL011_OUT_RING_EMPTY(intf) )
> >> + {
> >> + vpl011->uartfr &= ~(BUSY);
> >> + vpl011->uartfr |= (TXFE);
> >
> > It is not a good idea to check the intf values twice: xenconsoled might
> > be modifying them, so we can get into the situation where intf->out*
> > change in between the two checks. Please read out_cons and out_prod only
> > once in this function.
>
> Since this code is under LOCK, the IN and OUT ring buffers will not be
> updated by the guest. Specifically, the following transitions are
> ruled out:
>
> IN ring buffer
> non-empty ----> empty (as the reader is blocked due to lock)
>
> OUT ring buffer
> not-full ----> full (as writer is blocked due to lock).
>
> So the code inside the IF block remains valid even if the buffer state changes.
> For the IN ring buffer it can go from non-empty to full. Similarly for
> OUT ring buffer it can go from FULL to empty.
>
> Also checking the latest buffer index (instead of checking buffer
> index read as local variables) allows to update the pl011 state at the
> earliest.
I understand that the IN state shouldn't change. However, like you say,
the OUT state can change between the VPL011_OUT_RING_FULL and the
VPL011_OUT_RING_EMPTY checks.
It is a bad idea to try to write lockless code against a potentially
changing state. It is already hard enough without adding that
complexity; the other end can find ways to cause problems. Please take
a snapshot of the out indexes and use the local variables, rather than
"intf", to do the checks.
> >
> >
> >> + }
> >> + }
> >> +
> >> + VPL011_UNLOCK(d, flags);
> >> +
> >> + vgic_inject_vpl011_spi(d);
> >> +
> >> + if ( !VPL011_OUT_RING_EMPTY(intf) )
> >> + {
> >> + ASSERT( d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] != 0 );
> >> + notify_via_xen_event_channel(d, d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN]);
> >
> > Why are we sending notifications at this point? The out buffer hasn't
> > changed since xenconsoled sent the evtchn to Xen, so xenconsoled already
> > knows at this point that the out ring is not empty.
>
> The OUT buffer may be empty when xenconsoled sends the event. By the
> time the event is handled, the guest may write more data to OUT
> buffer. This condition makes sure that if the OUT ring buffer is not
> empty then it sends an event to xenconsoled.
OUT notifications from Xen to xenconsoled are best handled where writes
actually happen: in vpl011_write_data. I could accept an optimization
like the one you introduced, but please explain the design and the
benefits in details.
> >
> >
> >> + }
> >> +}
> >> +
> >> +
> >> +static void vpl011_notification(struct vcpu *v, unsigned int port)
> >> +{
> >> + vpl011_data_avail(v->domain);
> >> +}
> >> +
> >> +int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig *config)
> >> +{
> >> + int rc;
> >> + struct vpl011_s *vpl011 = &d->arch.vpl011;
> >> +
> >> + rc = alloc_unbound_xen_event_channel(d, 0, config->console_domid,
> >> + vpl011_notification);
> >> + if (rc < 0)
> >> + {
> >> + return rc;
> >> + }
> >> + d->arch.hvm_domain.params[HVM_PARAM_VCONSOLE_EVTCHN] = rc;
> >> + rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> >> + if ( !rc )
> >> + return rc;
> >
> > We should free the allocated evtchn in case of error here
> I will free the event channel.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-19 18:40 ` Stefano Stabellini
@ 2017-04-25 7:31 ` Bhupinder Thakur
2017-04-25 17:56 ` Stefano Stabellini
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-25 7:31 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Ian Jackson, Wei Liu
On 20 April 2017 at 00:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
> We don't have a formal protocol spec for PV console, but if we had, it
> would say that the frontend (Xen in this case) should send notifications
> every time there is new data to write. Thus, once per character in this
> case.
>
> I would start with a dumb implementation like that, simply notify the
> other end every time. Then, in a separate patch, we can consider
> introducing optimizations, which need to be well explained.
>
Ok. I will add this optimisation as a separate patch later.
>
> Regarding the optimization you introduced in this patch, delaying write
> notifications until we receive a notification from xenconsoled, how many
> notifications from xen to xenconsoled does it actually save? xenconsoled
> is going to send a notification for every read: we might end up sending
> the same number of notifications, only delayed.
>
>
In the PV console design, the events from the guest are sent to
xenconsole for every chunk of data. Since in the pl011 emulation, the
data comes in bytes only, it would generate lot of events to
xenconsole. To reduce the flurry of events, this optimisation was
added.
Xenconsole sends an event in the following conditions:
1. There is data available for Xen to process
2. It has finished processing the data and Xen can send more data
In the 2nd case, xenconsole will keep reading the data from the ring
buffer until it goes empty. At that point, it would send an event to
Xen. Between sending of this event and processing of this event by
Xen, there could be more data added for the xenconsole to process.
While handling an event, the Xen will check for that condition and if
there is data to be processed by xenconsole, it would send an event.
Also sending delayed events helps with the rate limit check in
xenconsole. If there are too many events, they maybe masked by
xenconsole. I could test whether this rate limit check is really
getting hit with and without this optimisation.
>>
>> Since this code is under LOCK, the IN and OUT ring buffers will not be
>> updated by the guest. Specifically, the following transitions are
>> ruled out:
>>
>> IN ring buffer
>> non-empty ----> empty (as the reader is blocked due to lock)
>>
>> OUT ring buffer
>> not-full ----> full (as writer is blocked due to lock).
>>
>> So the code inside the IF block remains valid even if the buffer state changes.
>> For the IN ring buffer it can go from non-empty to full. Similarly for
>> OUT ring buffer it can go from FULL to empty.
>>
>> Also checking the latest buffer index (instead of checking buffer
>> index read as local variables) allows to update the pl011 state at the
>> earliest.
>
> I understand that the IN state shouldn't change. However, like you say,
> the OUT state can change between the VPL011_OUT_RING_FULL and the
> VPL011_OUT_RING_EMPTY checks.
>
> It is a bad idea to try to write lockless code against a potentially
> changing state. It is already hard enough without adding that
> complexity; the other end can find ways to cause problems. Please take
> a snapshot of the out indexes and use the local variables, rather than
> "intf", to do the checks.
>
I will use local variables to do these checks.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-25 7:31 ` Bhupinder Thakur
@ 2017-04-25 17:56 ` Stefano Stabellini
2017-04-26 7:49 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-25 17:56 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Tue, 25 Apr 2017, Bhupinder Thakur wrote:
> On 20 April 2017 at 00:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > We don't have a formal protocol spec for PV console, but if we had, it
> > would say that the frontend (Xen in this case) should send notifications
> > every time there is new data to write. Thus, once per character in this
> > case.
> >
> > I would start with a dumb implementation like that, simply notify the
> > other end every time. Then, in a separate patch, we can consider
> > introducing optimizations, which need to be well explained.
> >
> Ok. I will add this optimisation as a separate patch later.
>
> >
> > Regarding the optimization you introduced in this patch, delaying write
> > notifications until we receive a notification from xenconsoled, how many
> > notifications from xen to xenconsoled does it actually save? xenconsoled
> > is going to send a notification for every read: we might end up sending
> > the same number of notifications, only delayed.
> >
> >
> In the PV console design, the events from the guest are sent to
> xenconsole for every chunk of data. Since in the pl011 emulation, the
> data comes in bytes only, it would generate lot of events to
> xenconsole. To reduce the flurry of events, this optimisation was
> added.
>
> Xenconsole sends an event in the following conditions:
>
> 1. There is data available for Xen to process
> 2. It has finished processing the data and Xen can send more data
>
> In the 2nd case, xenconsole will keep reading the data from the ring
> buffer until it goes empty. At that point, it would send an event to
> Xen. Between sending of this event and processing of this event by
> Xen, there could be more data added for the xenconsole to process.
> While handling an event, the Xen will check for that condition and if
> there is data to be processed by xenconsole, it would send an event.
>
> Also sending delayed events helps with the rate limit check in
> xenconsole. If there are too many events, they maybe masked by
> xenconsole. I could test whether this rate limit check is really
> getting hit with and without this optimisation.
I understand the idea behind, my question is whether this approach was
actually verified by any scientific measurements. Did you run a test
to count how many notifications were skipped thanks to this
optimization? If so, what was the scenario? How many notifications were
saved? If you didn't run a test, I suggest you do :-)
> >>
> >> Since this code is under LOCK, the IN and OUT ring buffers will not be
> >> updated by the guest. Specifically, the following transitions are
> >> ruled out:
> >>
> >> IN ring buffer
> >> non-empty ----> empty (as the reader is blocked due to lock)
> >>
> >> OUT ring buffer
> >> not-full ----> full (as writer is blocked due to lock).
> >>
> >> So the code inside the IF block remains valid even if the buffer state changes.
> >> For the IN ring buffer it can go from non-empty to full. Similarly for
> >> OUT ring buffer it can go from FULL to empty.
> >>
> >> Also checking the latest buffer index (instead of checking buffer
> >> index read as local variables) allows to update the pl011 state at the
> >> earliest.
> >
> > I understand that the IN state shouldn't change. However, like you say,
> > the OUT state can change between the VPL011_OUT_RING_FULL and the
> > VPL011_OUT_RING_EMPTY checks.
> >
> > It is a bad idea to try to write lockless code against a potentially
> > changing state. It is already hard enough without adding that
> > complexity; the other end can find ways to cause problems. Please take
> > a snapshot of the out indexes and use the local variables, rather than
> > "intf", to do the checks.
> >
> I will use local variables to do these checks.
>
>
> Regards,
> Bhupinder
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-25 17:56 ` Stefano Stabellini
@ 2017-04-26 7:49 ` Bhupinder Thakur
2017-04-26 8:13 ` Julien Grall
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-26 7:49 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Ian Jackson, Wei Liu
Hi Stefano,
>> > Regarding the optimization you introduced in this patch, delaying write
>> > notifications until we receive a notification from xenconsoled, how many
>> > notifications from xen to xenconsoled does it actually save? xenconsoled
>> > is going to send a notification for every read: we might end up sending
>> > the same number of notifications, only delayed.
>> >
>> >
>> In the PV console design, the events from the guest are sent to
>> xenconsole for every chunk of data. Since in the pl011 emulation, the
>> data comes in bytes only, it would generate lot of events to
>> xenconsole. To reduce the flurry of events, this optimisation was
>> added.
>>
>> Xenconsole sends an event in the following conditions:
>>
>> 1. There is data available for Xen to process
>> 2. It has finished processing the data and Xen can send more data
>>
>> In the 2nd case, xenconsole will keep reading the data from the ring
>> buffer until it goes empty. At that point, it would send an event to
>> Xen. Between sending of this event and processing of this event by
>> Xen, there could be more data added for the xenconsole to process.
>> While handling an event, the Xen will check for that condition and if
>> there is data to be processed by xenconsole, it would send an event.
>>
>> Also sending delayed events helps with the rate limit check in
>> xenconsole. If there are too many events, they maybe masked by
>> xenconsole. I could test whether this rate limit check is really
>> getting hit with and without this optimisation.
>
> I understand the idea behind, my question is whether this approach was
> actually verified by any scientific measurements. Did you run a test
> to count how many notifications were skipped thanks to this
> optimization? If so, what was the scenario? How many notifications were
> saved? If you didn't run a test, I suggest you do :-)
>
Today I did some instrumentation and count the number of events sent
by Xen to xenconsole and how many are really processed by xenconsole.
I could not see any difference in the number of events processed by
xenconsole with or without the optimization. The total number of
events processed by xenconsole were about 500 for the complete guest
booting till the login prompt (for both optimised and non-optimised
case).
Although Xen calls notify_via_xen_event_channel() far more number of
times (about 12000 times until the guest loging prompt comes) without
the optimisation, it does not translate into sending those many events
to xenconsole though. With the optmization it just reduces the number
of times notify_via_xen_event_channel() is called which is about 500
times.
I believe the reason could be that if the event is still pending with
xenconsole when the next event comes via
notify_via_xen_event_channel() then all such events would be coalesced
and delivered to xenconsole as a single event.
So the optimization does not help with saving any processing on
xenconsole though it saves the overhead of calling
notify_via_xen_event_channel() very frequently in Xen.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-26 7:49 ` Bhupinder Thakur
@ 2017-04-26 8:13 ` Julien Grall
2017-04-26 17:03 ` Stefano Stabellini
0 siblings, 1 reply; 53+ messages in thread
From: Julien Grall @ 2017-04-26 8:13 UTC (permalink / raw)
To: Bhupinder Thakur, Stefano Stabellini; +Cc: xen-devel, nd, Ian Jackson, Wei Liu
Hi Bhupinder,
On 26/04/2017 08:49, Bhupinder Thakur wrote:
>>>> Regarding the optimization you introduced in this patch, delaying write
>>>> notifications until we receive a notification from xenconsoled, how many
>>>> notifications from xen to xenconsoled does it actually save? xenconsoled
>>>> is going to send a notification for every read: we might end up sending
>>>> the same number of notifications, only delayed.
>>>>
>>>>
>>> In the PV console design, the events from the guest are sent to
>>> xenconsole for every chunk of data. Since in the pl011 emulation, the
>>> data comes in bytes only, it would generate lot of events to
>>> xenconsole. To reduce the flurry of events, this optimisation was
>>> added.
>>>
>>> Xenconsole sends an event in the following conditions:
>>>
>>> 1. There is data available for Xen to process
>>> 2. It has finished processing the data and Xen can send more data
>>>
>>> In the 2nd case, xenconsole will keep reading the data from the ring
>>> buffer until it goes empty. At that point, it would send an event to
>>> Xen. Between sending of this event and processing of this event by
>>> Xen, there could be more data added for the xenconsole to process.
>>> While handling an event, the Xen will check for that condition and if
>>> there is data to be processed by xenconsole, it would send an event.
>>>
>>> Also sending delayed events helps with the rate limit check in
>>> xenconsole. If there are too many events, they maybe masked by
>>> xenconsole. I could test whether this rate limit check is really
>>> getting hit with and without this optimisation.
>>
>> I understand the idea behind, my question is whether this approach was
>> actually verified by any scientific measurements. Did you run a test
>> to count how many notifications were skipped thanks to this
>> optimization? If so, what was the scenario? How many notifications were
>> saved? If you didn't run a test, I suggest you do :-)
>>
> Today I did some instrumentation and count the number of events sent
> by Xen to xenconsole and how many are really processed by xenconsole.
>
> I could not see any difference in the number of events processed by
> xenconsole with or without the optimization. The total number of
> events processed by xenconsole were about 500 for the complete guest
> booting till the login prompt (for both optimised and non-optimised
> case).
>
> Although Xen calls notify_via_xen_event_channel() far more number of
> times (about 12000 times until the guest loging prompt comes) without
> the optimisation, it does not translate into sending those many events
> to xenconsole though. With the optmization it just reduces the number
> of times notify_via_xen_event_channel() is called which is about 500
> times.
>
> I believe the reason could be that if the event is still pending with
> xenconsole when the next event comes via
> notify_via_xen_event_channel() then all such events would be coalesced
> and delivered to xenconsole as a single event.
>
> So the optimization does not help with saving any processing on
> xenconsole though it saves the overhead of calling
> notify_via_xen_event_channel() very frequently in Xen.
I don't see any issue to call notify_via_xen_event_channel many time
because you should do it for every batch sent.
Yes, the batch consists only of one character, but this is how an UART
is designed.
You rely on the behavior of notify_via_xen_event_channel. What if in the
future it changes? Then maybe, you will miss event and data.
So I would rather avoid this premature optimization and see how it is
behaving. If it is too slow, then we can think about it.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation in Xen
2017-04-26 8:13 ` Julien Grall
@ 2017-04-26 17:03 ` Stefano Stabellini
0 siblings, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-26 17:03 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Bhupinder Thakur,
xen-devel, nd
On Wed, 26 Apr 2017, Julien Grall wrote:
> Hi Bhupinder,
>
> On 26/04/2017 08:49, Bhupinder Thakur wrote:
> > > > > Regarding the optimization you introduced in this patch, delaying
> > > > > write
> > > > > notifications until we receive a notification from xenconsoled, how
> > > > > many
> > > > > notifications from xen to xenconsoled does it actually save?
> > > > > xenconsoled
> > > > > is going to send a notification for every read: we might end up
> > > > > sending
> > > > > the same number of notifications, only delayed.
> > > > >
> > > > >
> > > > In the PV console design, the events from the guest are sent to
> > > > xenconsole for every chunk of data. Since in the pl011 emulation, the
> > > > data comes in bytes only, it would generate lot of events to
> > > > xenconsole. To reduce the flurry of events, this optimisation was
> > > > added.
> > > >
> > > > Xenconsole sends an event in the following conditions:
> > > >
> > > > 1. There is data available for Xen to process
> > > > 2. It has finished processing the data and Xen can send more data
> > > >
> > > > In the 2nd case, xenconsole will keep reading the data from the ring
> > > > buffer until it goes empty. At that point, it would send an event to
> > > > Xen. Between sending of this event and processing of this event by
> > > > Xen, there could be more data added for the xenconsole to process.
> > > > While handling an event, the Xen will check for that condition and if
> > > > there is data to be processed by xenconsole, it would send an event.
> > > >
> > > > Also sending delayed events helps with the rate limit check in
> > > > xenconsole. If there are too many events, they maybe masked by
> > > > xenconsole. I could test whether this rate limit check is really
> > > > getting hit with and without this optimisation.
> > >
> > > I understand the idea behind, my question is whether this approach was
> > > actually verified by any scientific measurements. Did you run a test
> > > to count how many notifications were skipped thanks to this
> > > optimization? If so, what was the scenario? How many notifications were
> > > saved? If you didn't run a test, I suggest you do :-)
> > >
> > Today I did some instrumentation and count the number of events sent
> > by Xen to xenconsole and how many are really processed by xenconsole.
> >
> > I could not see any difference in the number of events processed by
> > xenconsole with or without the optimization. The total number of
> > events processed by xenconsole were about 500 for the complete guest
> > booting till the login prompt (for both optimised and non-optimised
> > case).
> >
> > Although Xen calls notify_via_xen_event_channel() far more number of
> > times (about 12000 times until the guest loging prompt comes) without
> > the optimisation, it does not translate into sending those many events
> > to xenconsole though. With the optmization it just reduces the number
> > of times notify_via_xen_event_channel() is called which is about 500
> > times.
> >
> > I believe the reason could be that if the event is still pending with
> > xenconsole when the next event comes via
> > notify_via_xen_event_channel() then all such events would be coalesced
> > and delivered to xenconsole as a single event.
> >
> > So the optimization does not help with saving any processing on
> > xenconsole though it saves the overhead of calling
> > notify_via_xen_event_channel() very frequently in Xen.
Thank you Bhupinder for doing the test! This is why I always ask to do
performance measurements: changes that look obvious don't always
translate in the optimizations that we hope for.
> I don't see any issue to call notify_via_xen_event_channel many time because
> you should do it for every batch sent.
>
> Yes, the batch consists only of one character, but this is how an UART is
> designed.
>
> You rely on the behavior of notify_via_xen_event_channel. What if in the
> future it changes? Then maybe, you will miss event and data.
>
> So I would rather avoid this premature optimization and see how it is
> behaving. If it is too slow, then we can think about it.
I agree with Julien. Just leave it without optimizations for now. We can
work on this separately.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 02/10] xen/arm: vpl011: Add new virtual console hvm params in Xen
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
2017-04-03 9:44 ` [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation " Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-19 0:22 ` Stefano Stabellini
2017-04-03 9:44 ` [PATCH 03/10] xen/arm: vpl011: Enable pl011 emulation for a guest domain " Bhupinder Thakur
` (8 subsequent siblings)
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
1. Add two new HVM param handlers for:
- Allocate a new event channel for sending/receiving events to/from Xen.
- Map the PFN allocted by the toolstack to be used as IN/OUT ring buffers.
2. Add validation to disallow get/set of these HVM params from guest
domain.
Xen will communicate with xenconsole over the ring buffer and the event
channel to transmit and receive pl011 data on guest domain's behalf.
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
xen/arch/arm/hvm.c | 112 ++++++++++++++++++++++++++++++++++++++++
xen/include/public/hvm/params.h | 10 ++++
2 files changed, 122 insertions(+)
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index d999bde..c1fed45 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -23,6 +23,8 @@
#include <xen/guest_access.h>
#include <xen/sched.h>
#include <xen/monitor.h>
+#include <xen/event.h>
+#include <xen/vmap.h>
#include <xsm/xsm.h>
@@ -31,6 +33,80 @@
#include <public/hvm/hvm_op.h>
#include <asm/hypercall.h>
+#include <xen/vpl011.h>
+
+static bool vpl011_built(void)
+{
+#ifdef CONFIG_VPL011_CONSOLE
+ return true;
+#else
+ return false;
+#endif
+}
+
+static int hvm_allow_set_param(struct domain *d,
+ const struct xen_hvm_param *a)
+{
+ uint64_t value = d->arch.hvm_domain.params[a->index];
+ int rc;
+
+ rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+ if ( rc )
+ return rc;
+
+ switch ( a->index )
+ {
+ /* The following parameters should not be set by the guest. */
+ case HVM_PARAM_VCONSOLE_PFN:
+ case HVM_PARAM_VCONSOLE_EVTCHN:
+ if ( d == current->domain )
+ rc = -EPERM;
+ break;
+ default:
+ break;
+ }
+
+ if ( rc )
+ return rc;
+
+ switch ( a->index )
+ {
+ /* The following parameters should only be changed once. */
+ case HVM_PARAM_VCONSOLE_PFN:
+ case HVM_PARAM_VCONSOLE_EVTCHN:
+ if ( value != 0 && a->value != value )
+ rc = -EEXIST;
+ break;
+ default:
+ break;
+ }
+
+ return rc;
+}
+
+static int hvm_allow_get_param(struct domain *d,
+ const struct xen_hvm_param *a)
+{
+ int rc;
+
+ rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+ if ( rc )
+ return rc;
+
+ switch ( a->index )
+ {
+ /* The remaining parameters should not be read by the guest. */
+ case HVM_PARAM_VCONSOLE_PFN:
+ case HVM_PARAM_VCONSOLE_EVTCHN:
+ if ( d == current->domain )
+ rc = -EPERM;
+ break;
+ default:
+ break;
+ }
+
+ return rc;
+}
long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
{
@@ -61,9 +137,45 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( op == HVMOP_set_param )
{
d->arch.hvm_domain.params[a.index] = a.value;
+
+ if ( a.index == HVM_PARAM_VCONSOLE_PFN ||
+ a.index == HVM_PARAM_VCONSOLE_EVTCHN )
+ {
+ if ( vpl011_built() )
+ {
+ rc = hvm_allow_set_param(d, &a);
+ if ( rc )
+ goto param_fail;
+
+ if ( a.index == HVM_PARAM_VCONSOLE_PFN )
+ {
+ rc = vpl011_map_guest_page(d);
+ if ( rc )
+ goto param_fail;
+ }
+ }
+ else
+ {
+ rc = -1;
+ goto param_fail;
+ }
+ }
}
else
{
+ if ( a.index == HVM_PARAM_VCONSOLE_PFN ||
+ a.index == HVM_PARAM_VCONSOLE_EVTCHN )
+ {
+ if ( !vpl011_built() )
+ {
+ rc = -1;
+ goto param_fail;
+ }
+ }
+ rc = hvm_allow_get_param(d, &a);
+ if ( rc )
+ goto param_fail;
+
a.value = d->arch.hvm_domain.params[a.index];
rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
}
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 3f54a49..15d37e5 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -203,10 +203,20 @@
*/
#define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
+#if defined(__arm__) || defined(__aarch64__)
+/* Virtual console (VC) shared memory ring and event channel. */
+#define HVM_PARAM_VCONSOLE_PFN 20
+#define HVM_PARAM_VCONSOLE_EVTCHN 21
+#else
/* Deprecated */
#define HVM_PARAM_MEMORY_EVENT_CR0 20
#define HVM_PARAM_MEMORY_EVENT_CR3 21
+#endif
+
+/* Deprecated */
#define HVM_PARAM_MEMORY_EVENT_CR4 22
+
+/* Deprecated */
#define HVM_PARAM_MEMORY_EVENT_INT3 23
#define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP 25
#define HVM_PARAM_MEMORY_EVENT_MSR 30
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 02/10] xen/arm: vpl011: Add new virtual console hvm params in Xen
2017-04-03 9:44 ` [PATCH 02/10] xen/arm: vpl011: Add new virtual console hvm params " Bhupinder Thakur
@ 2017-04-19 0:22 ` Stefano Stabellini
2017-04-19 8:48 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 0:22 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> 1. Add two new HVM param handlers for:
> - Allocate a new event channel for sending/receiving events to/from Xen.
> - Map the PFN allocted by the toolstack to be used as IN/OUT ring buffers.
>
> 2. Add validation to disallow get/set of these HVM params from guest
> domain.
>
> Xen will communicate with xenconsole over the ring buffer and the event
> channel to transmit and receive pl011 data on guest domain's behalf.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> xen/arch/arm/hvm.c | 112 ++++++++++++++++++++++++++++++++++++++++
> xen/include/public/hvm/params.h | 10 ++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index d999bde..c1fed45 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -23,6 +23,8 @@
> #include <xen/guest_access.h>
> #include <xen/sched.h>
> #include <xen/monitor.h>
> +#include <xen/event.h>
> +#include <xen/vmap.h>
>
> #include <xsm/xsm.h>
>
> @@ -31,6 +33,80 @@
> #include <public/hvm/hvm_op.h>
>
> #include <asm/hypercall.h>
> +#include <xen/vpl011.h>
> +
> +static bool vpl011_built(void)
> +{
> +#ifdef CONFIG_VPL011_CONSOLE
> + return true;
> +#else
> + return false;
> +#endif
> +}
> +
> +static int hvm_allow_set_param(struct domain *d,
> + const struct xen_hvm_param *a)
> +{
> + uint64_t value = d->arch.hvm_domain.params[a->index];
> + int rc;
> +
> + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
> + if ( rc )
> + return rc;
> +
> + switch ( a->index )
> + {
> + /* The following parameters should not be set by the guest. */
> + case HVM_PARAM_VCONSOLE_PFN:
> + case HVM_PARAM_VCONSOLE_EVTCHN:
> + if ( d == current->domain )
> + rc = -EPERM;
> + break;
> + default:
> + break;
> + }
> +
> + if ( rc )
> + return rc;
> +
> + switch ( a->index )
> + {
> + /* The following parameters should only be changed once. */
> + case HVM_PARAM_VCONSOLE_PFN:
> + case HVM_PARAM_VCONSOLE_EVTCHN:
> + if ( value != 0 && a->value != value )
> + rc = -EEXIST;
> + break;
> + default:
> + break;
> + }
> +
> + return rc;
> +}
> +
> +static int hvm_allow_get_param(struct domain *d,
> + const struct xen_hvm_param *a)
> +{
> + int rc;
> +
> + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
> + if ( rc )
> + return rc;
> +
> + switch ( a->index )
> + {
> + /* The remaining parameters should not be read by the guest. */
> + case HVM_PARAM_VCONSOLE_PFN:
> + case HVM_PARAM_VCONSOLE_EVTCHN:
> + if ( d == current->domain )
> + rc = -EPERM;
> + break;
> + default:
> + break;
> + }
> +
> + return rc;
> +}
>
> long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> @@ -61,9 +137,45 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( op == HVMOP_set_param )
> {
> d->arch.hvm_domain.params[a.index] = a.value;
> +
> + if ( a.index == HVM_PARAM_VCONSOLE_PFN ||
> + a.index == HVM_PARAM_VCONSOLE_EVTCHN )
> + {
> + if ( vpl011_built() )
Code style: we only use spaces for indentation in Xen. Please fix across
the series.
Also, I would move the "if ( vpl011_built() )" check to
hvm_allow_set_param and hvm_allow_get_param.
> + {
> + rc = hvm_allow_set_param(d, &a);
> + if ( rc )
> + goto param_fail;
> +
> + if ( a.index == HVM_PARAM_VCONSOLE_PFN )
> + {
> + rc = vpl011_map_guest_page(d);
> + if ( rc )
> + goto param_fail;
> + }
> + }
> + else
> + {
> + rc = -1;
> + goto param_fail;
> + }
> + }
> }
> else
> {
> + if ( a.index == HVM_PARAM_VCONSOLE_PFN ||
> + a.index == HVM_PARAM_VCONSOLE_EVTCHN )
> + {
> + if ( !vpl011_built() )
> + {
> + rc = -1;
> + goto param_fail;
> + }
> + }
> + rc = hvm_allow_get_param(d, &a);
> + if ( rc )
> + goto param_fail;
> +
> a.value = d->arch.hvm_domain.params[a.index];
> rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> }
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 3f54a49..15d37e5 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -203,10 +203,20 @@
> */
> #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>
> +#if defined(__arm__) || defined(__aarch64__)
> +/* Virtual console (VC) shared memory ring and event channel. */
> +#define HVM_PARAM_VCONSOLE_PFN 20
> +#define HVM_PARAM_VCONSOLE_EVTCHN 21
> +#else
> /* Deprecated */
> #define HVM_PARAM_MEMORY_EVENT_CR0 20
> #define HVM_PARAM_MEMORY_EVENT_CR3 21
> +#endif
> +
> +/* Deprecated */
> #define HVM_PARAM_MEMORY_EVENT_CR4 22
> +
> +/* Deprecated */
> #define HVM_PARAM_MEMORY_EVENT_INT3 23
> #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP 25
> #define HVM_PARAM_MEMORY_EVENT_MSR 30
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 02/10] xen/arm: vpl011: Add new virtual console hvm params in Xen
2017-04-19 0:22 ` Stefano Stabellini
@ 2017-04-19 8:48 ` Bhupinder Thakur
0 siblings, 0 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-19 8:48 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Ian Jackson, Wei Liu
Hi Stefano,
On 19 April 2017 at 05:52, Stefano Stabellini <sstabellini@kernel.org> wrote:
>> long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>> {
>> @@ -61,9 +137,45 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>> if ( op == HVMOP_set_param )
>> {
>> d->arch.hvm_domain.params[a.index] = a.value;
>> +
>> + if ( a.index == HVM_PARAM_VCONSOLE_PFN ||
>> + a.index == HVM_PARAM_VCONSOLE_EVTCHN )
>> + {
>> + if ( vpl011_built() )
>
> Code style: we only use spaces for indentation in Xen. Please fix across
> the series.
>
I think this was introduced by mistake as I had to keep switching the
tab settings while modifying Xen code and xenconsoled code. I will fix
it.
> Also, I would move the "if ( vpl011_built() )" check to
> hvm_allow_set_param and hvm_allow_get_param.
I will move vpl011_built() inside the hvm_allow functions.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 03/10] xen/arm: vpl011: Enable pl011 emulation for a guest domain in Xen
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
2017-04-03 9:44 ` [PATCH 01/10] xen/arm: vpl011: Add pl011 uart emulation " Bhupinder Thakur
2017-04-03 9:44 ` [PATCH 02/10] xen/arm: vpl011: Add new virtual console hvm params " Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-19 0:27 ` Stefano Stabellini
2017-04-03 9:44 ` [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation Bhupinder Thakur
` (7 subsequent siblings)
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
Vpl011 emulation is enabled for a guest domain in Xen only when it is
enabled through an option in libxl provided by the user through
guest configuration.
The pl011 enable/disable knob in libxl is introduced in the following
patch:
xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011
emulation
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
xen/arch/arm/domain.c | 11 +++++++++++
xen/common/domctl.c | 3 +++
xen/include/public/domctl.h | 2 ++
xen/include/xen/sched.h | 4 ++++
4 files changed, 20 insertions(+)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7e43691..8e383d0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -36,6 +36,9 @@
#include <asm/platform.h>
#include "vtimer.h"
#include "vuart.h"
+#ifdef CONFIG_VPL011_CONSOLE
+#include <xen/vpl011.h>
+#endif
DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
@@ -626,6 +629,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
if ( (rc = domain_vtimer_init(d, config)) != 0 )
goto fail;
+ if ( domcr_flags & DOMCRF_vconsole )
+#ifdef CONFIG_VPL011_CONSOLE
+ if ( (rc = domain_vpl011_init(d, config)) != 0 )
+#endif
+ goto fail;
update_domain_wallclock_time(d);
/*
@@ -660,6 +668,9 @@ fail:
void arch_domain_destroy(struct domain *d)
{
+#ifdef CONFIG_VPL011_CONSOLE
+ domain_vpl011_deinit(d);
+#endif
/* IOMMU page table is shared with P2M, always call
* iommu_domain_destroy() before p2m_teardown().
*/
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 12cf4a9..3385479 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -506,6 +506,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
| XEN_DOMCTL_CDF_hap
| XEN_DOMCTL_CDF_s3_integrity
| XEN_DOMCTL_CDF_oos_off
+ | XEN_DOMCTL_VCONSOLE_enable
| XEN_DOMCTL_CDF_xs_domain)) )
break;
@@ -550,6 +551,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
domcr_flags |= DOMCRF_oos_off;
if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
domcr_flags |= DOMCRF_xs_domain;
+ if ( op->u.createdomain.flags & XEN_DOMCTL_VCONSOLE_enable )
+ domcr_flags |= DOMCRF_vconsole;
d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
&op->u.createdomain.config);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 85cbb7c..1fca180 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -66,6 +66,8 @@ struct xen_domctl_createdomain {
/* Is this a xenstore domain? */
#define _XEN_DOMCTL_CDF_xs_domain 5
#define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain)
+#define _XEN_DOMCTL_VCONSOLE_enable 6
+#define XEN_DOMCTL_VCONSOLE_enable (1U<<_XEN_DOMCTL_VCONSOLE_enable)
uint32_t flags;
struct xen_arch_domainconfig config;
};
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 063efe6..4efdc48 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -555,6 +555,10 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
#define _DOMCRF_xs_domain 6
#define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain)
+ /* DOMCRF_vconsole: enable virtual console emulation. Used for aarach64. */
+#define _DOMCRF_vconsole 7
+#define DOMCRF_vconsole (1U<<_DOMCRF_vconsole)
+
/*
* rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
* This is the preferred function if the returned domain reference
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 03/10] xen/arm: vpl011: Enable pl011 emulation for a guest domain in Xen
2017-04-03 9:44 ` [PATCH 03/10] xen/arm: vpl011: Enable pl011 emulation for a guest domain " Bhupinder Thakur
@ 2017-04-19 0:27 ` Stefano Stabellini
0 siblings, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 0:27 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> Vpl011 emulation is enabled for a guest domain in Xen only when it is
> enabled through an option in libxl provided by the user through
> guest configuration.
>
> The pl011 enable/disable knob in libxl is introduced in the following
> patch:
> xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011
> emulation
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> xen/arch/arm/domain.c | 11 +++++++++++
> xen/common/domctl.c | 3 +++
> xen/include/public/domctl.h | 2 ++
> xen/include/xen/sched.h | 4 ++++
> 4 files changed, 20 insertions(+)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7e43691..8e383d0 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -36,6 +36,9 @@
> #include <asm/platform.h>
> #include "vtimer.h"
> #include "vuart.h"
> +#ifdef CONFIG_VPL011_CONSOLE
> +#include <xen/vpl011.h>
> +#endif
>
> DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>
> @@ -626,6 +629,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> if ( (rc = domain_vtimer_init(d, config)) != 0 )
> goto fail;
>
> + if ( domcr_flags & DOMCRF_vconsole )
> +#ifdef CONFIG_VPL011_CONSOLE
> + if ( (rc = domain_vpl011_init(d, config)) != 0 )
> +#endif
> + goto fail;
Ideally, the #ifdef CONFIG_VPL011_CONSOLE would be confined within
xen/vpl011.h, and #ifndef CONFIG_VPL011_CONSOLE:
static inline int domain_vpl011_init(struct domain *d, ...) { return -ENOSYS; }
> update_domain_wallclock_time(d);
>
> /*
> @@ -660,6 +668,9 @@ fail:
>
> void arch_domain_destroy(struct domain *d)
> {
> +#ifdef CONFIG_VPL011_CONSOLE
> + domain_vpl011_deinit(d);
> +#endif
Same here, in vpl011.h:
static inline void domain_vpl011_deinit(struct domain *d) { }
> /* IOMMU page table is shared with P2M, always call
> * iommu_domain_destroy() before p2m_teardown().
> */
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 12cf4a9..3385479 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -506,6 +506,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> | XEN_DOMCTL_CDF_hap
> | XEN_DOMCTL_CDF_s3_integrity
> | XEN_DOMCTL_CDF_oos_off
> + | XEN_DOMCTL_VCONSOLE_enable
> | XEN_DOMCTL_CDF_xs_domain)) )
> break;
>
> @@ -550,6 +551,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> domcr_flags |= DOMCRF_oos_off;
> if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
> domcr_flags |= DOMCRF_xs_domain;
> + if ( op->u.createdomain.flags & XEN_DOMCTL_VCONSOLE_enable )
> + domcr_flags |= DOMCRF_vconsole;
>
> d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
> &op->u.createdomain.config);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 85cbb7c..1fca180 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -66,6 +66,8 @@ struct xen_domctl_createdomain {
> /* Is this a xenstore domain? */
> #define _XEN_DOMCTL_CDF_xs_domain 5
> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain)
> +#define _XEN_DOMCTL_VCONSOLE_enable 6
> +#define XEN_DOMCTL_VCONSOLE_enable (1U<<_XEN_DOMCTL_VCONSOLE_enable)
> uint32_t flags;
> struct xen_arch_domainconfig config;
> };
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 063efe6..4efdc48 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -555,6 +555,10 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
> #define _DOMCRF_xs_domain 6
> #define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain)
>
> + /* DOMCRF_vconsole: enable virtual console emulation. Used for aarach64. */
typo: aarch64
> +#define _DOMCRF_vconsole 7
> +#define DOMCRF_vconsole (1U<<_DOMCRF_vconsole)
> +
> /*
> * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
> * This is the preferred function if the returned domain reference
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (2 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 03/10] xen/arm: vpl011: Enable pl011 emulation for a guest domain " Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-12 16:32 ` Wei Liu
2017-04-03 9:44 ` [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console Bhupinder Thakur
` (6 subsequent siblings)
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
An option is provided in libxl to enable/disable pl011 emulation while
creating a guest domain.
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/libxl/libxl_create.c | 12 ++++++++++++
tools/libxl/libxl_internal.h | 5 +++++
tools/libxl/libxl_types.idl | 2 ++
tools/libxl/xl_cmdimpl.c | 4 ++++
4 files changed, 23 insertions(+)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e3bc257..9a59354 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -208,6 +208,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->disable_migrate, false);
+ libxl_defbool_setdefault(&b_info->enable_pl011, false);
+
for (i = 0 ; i < b_info->num_iomem; i++)
if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
b_info->iomem[i].gfn = b_info->iomem[i].start;
@@ -546,6 +548,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
flags |= XEN_DOMCTL_CDF_hap;
}
+ if (libxl_defbool_val(d_config->b_info.enable_pl011))
+ flags |= XEN_DOMCTL_VCONSOLE_enable;
+
/* Ultimately, handle is an array of 16 uint8_t, same as uuid */
libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
@@ -910,6 +915,11 @@ static void initiate_domain_create(libxl__egc *egc,
goto error_out;
}
+ if (libxl_defbool_val(d_config->b_info.enable_pl011))
+ state->vconsole_enabled = true;
+ else
+ state->vconsole_enabled = false;
+
if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
(libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {
@@ -926,6 +936,8 @@ static void initiate_domain_create(libxl__egc *egc,
goto error_out;
}
+ state->config.console_domid = state->console_domid;
+
ret = libxl__domain_make(gc, d_config, &domid, &state->config);
if (ret) {
LOGD(ERROR, domid, "cannot make domain: %d", ret);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5f46578..2406eaa 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1128,6 +1128,11 @@ typedef struct {
uint32_t num_vmemranges;
xc_domain_configuration_t config;
+
+ /* Virtual console mfn and port. */
+ unsigned long vconsole_mfn;
+ uint32_t vconsole_port;
+ bool vconsole_enabled;
} libxl__domain_build_state;
_hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a612d1f..fe7f795 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -105,6 +105,7 @@ libxl_console_type = Enumeration("console_type", [
(0, "UNKNOWN"),
(1, "SERIAL"),
(2, "PV"),
+ (3, "VCON"),
])
libxl_disk_format = Enumeration("disk_format", [
@@ -460,6 +461,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("disable_migrate", libxl_defbool),
("cpuid", libxl_cpuid_policy_list),
("blkdev_start", string),
+ ("enable_pl011", libxl_defbool),
("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 358757f..4f4d4e6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1433,6 +1433,8 @@ static void parse_config_data(const char *config_source,
if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
b_info->max_vcpus = l;
+ xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011, 0);
+
parse_vnuma_config(config, b_info);
/* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
@@ -3788,6 +3790,8 @@ int main_console(int argc, char **argv)
type = LIBXL_CONSOLE_TYPE_PV;
else if (!strcmp(optarg, "serial"))
type = LIBXL_CONSOLE_TYPE_SERIAL;
+ else if (!strcmp(optarg, "vcon"))
+ type = LIBXL_CONSOLE_TYPE_VCON;
else {
fprintf(stderr, "console type supported are: pv, serial\n");
return EXIT_FAILURE;
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-03 9:44 ` [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation Bhupinder Thakur
@ 2017-04-12 16:32 ` Wei Liu
2017-04-13 8:19 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-12 16:32 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, Apr 03, 2017 at 03:14:27PM +0530, Bhupinder Thakur wrote:
[...]
> _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index a612d1f..fe7f795 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -105,6 +105,7 @@ libxl_console_type = Enumeration("console_type", [
> (0, "UNKNOWN"),
> (1, "SERIAL"),
> (2, "PV"),
> + (3, "VCON"),
> ])
>
You need to add a LIBXL_HAVE macro to libxl.h.
> libxl_disk_format = Enumeration("disk_format", [
> @@ -460,6 +461,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("disable_migrate", libxl_defbool),
> ("cpuid", libxl_cpuid_policy_list),
> ("blkdev_start", string),
> + ("enable_pl011", libxl_defbool),
>
> ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 358757f..4f4d4e6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1433,6 +1433,8 @@ static void parse_config_data(const char *config_source,
> if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
> b_info->max_vcpus = l;
>
> + xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011, 0);
> +
I'm not very keen on having "pl011" because it is ARM specific.
Is there a case in which you don't want this vconsole? Should we always
enable it in toolstack?
> parse_vnuma_config(config, b_info);
>
> /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
> @@ -3788,6 +3790,8 @@ int main_console(int argc, char **argv)
> type = LIBXL_CONSOLE_TYPE_PV;
> else if (!strcmp(optarg, "serial"))
> type = LIBXL_CONSOLE_TYPE_SERIAL;
> + else if (!strcmp(optarg, "vcon"))
> + type = LIBXL_CONSOLE_TYPE_VCON;
> else {
> fprintf(stderr, "console type supported are: pv, serial\n");
> return EXIT_FAILURE;
You also need to patch manpage etc.
It appears that your tree is not up to date -- xl was split out from
libxl at some point.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-12 16:32 ` Wei Liu
@ 2017-04-13 8:19 ` Bhupinder Thakur
2017-04-13 8:37 ` Wei Liu
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-13 8:19 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson
Hi Wei,
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index a612d1f..fe7f795 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -105,6 +105,7 @@ libxl_console_type = Enumeration("console_type", [
>> (0, "UNKNOWN"),
>> (1, "SERIAL"),
>> (2, "PV"),
>> + (3, "VCON"),
>> ])
>>
>
> You need to add a LIBXL_HAVE macro to libxl.h.
I will add this macro. I am trying to understand the use of these
macros. Are the applications which are building against a particular
libxl version supposed to check this flag before using this feature?
>
>> libxl_disk_format = Enumeration("disk_format", [
>> @@ -460,6 +461,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> ("disable_migrate", libxl_defbool),
>> ("cpuid", libxl_cpuid_policy_list),
>> ("blkdev_start", string),
>> + ("enable_pl011", libxl_defbool),
>>
>> ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 358757f..4f4d4e6 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1433,6 +1433,8 @@ static void parse_config_data(const char *config_source,
>> if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
>> b_info->max_vcpus = l;
>>
>> + xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011, 0);
>> +
>
> I'm not very keen on having "pl011" because it is ARM specific.
I will rename it to vconsole, which I am using through out the libxl code now.
>
> Is there a case in which you don't want this vconsole? Should we always
> enable it in toolstack?
There was a review comment from Julien that pl011 emulation should not
be enabled by default for domU. So based on this flag the pl011
emulation will be enabled/disabled at the time of domain creation.
>
>> parse_vnuma_config(config, b_info);
>>
>> /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
>> @@ -3788,6 +3790,8 @@ int main_console(int argc, char **argv)
>> type = LIBXL_CONSOLE_TYPE_PV;
>> else if (!strcmp(optarg, "serial"))
>> type = LIBXL_CONSOLE_TYPE_SERIAL;
>> + else if (!strcmp(optarg, "vcon"))
>> + type = LIBXL_CONSOLE_TYPE_VCON;
>> else {
>> fprintf(stderr, "console type supported are: pv, serial\n");
>> return EXIT_FAILURE;
>
> You also need to patch manpage etc.
>
> It appears that your tree is not up to date -- xl was split out from
> libxl at some point.
I will rebase my patches on the latest master branch.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-13 8:19 ` Bhupinder Thakur
@ 2017-04-13 8:37 ` Wei Liu
2017-04-19 0:29 ` Stefano Stabellini
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-13 8:37 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson
On Thu, Apr 13, 2017 at 01:49:51PM +0530, Bhupinder Thakur wrote:
> Hi Wei,
>
> >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> index a612d1f..fe7f795 100644
> >> --- a/tools/libxl/libxl_types.idl
> >> +++ b/tools/libxl/libxl_types.idl
> >> @@ -105,6 +105,7 @@ libxl_console_type = Enumeration("console_type", [
> >> (0, "UNKNOWN"),
> >> (1, "SERIAL"),
> >> (2, "PV"),
> >> + (3, "VCON"),
> >> ])
> >>
> >
> > You need to add a LIBXL_HAVE macro to libxl.h.
>
> I will add this macro. I am trying to understand the use of these
> macros. Are the applications which are building against a particular
> libxl version supposed to check this flag before using this feature?
>
Yes. Think about a new application that is built against an older
version of libxl source code that doesn't have this flag. It can use
this flag to check if such feature is available.
> >
> >> libxl_disk_format = Enumeration("disk_format", [ @@ -460,6 +461,7
> >> @@ libxl_domain_build_info = Struct("domain_build_info",[
> >> ("disable_migrate", libxl_defbool), ("cpuid",
> >> libxl_cpuid_policy_list), ("blkdev_start", string), +
> >> ("enable_pl011", libxl_defbool),
> >>
> >> ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 358757f..4f4d4e6 100644 --- a/tools/libxl/xl_cmdimpl.c +++
> >> b/tools/libxl/xl_cmdimpl.c @@ -1433,6 +1433,8 @@ static void
> >> parse_config_data(const char *config_source, if (!xlu_cfg_get_long
> >> (config, "maxvcpus", &l, 0)) b_info->max_vcpus = l;
> >>
> >> + xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011,
> >> 0); +
> >
> > I'm not very keen on having "pl011" because it is ARM specific.
> I will rename it to vconsole, which I am using through out the libxl
> code now.
Before you go away and change your code, we need to discuss this a bit.
Since there is no "console" option in xl, I think using just "console"
should be fine. And then user can specify "console=pl011" in their
config file.
There will also need to be changes in the libxl idl struct. If you
really want to use enable_pl011 (I would suggest to just use pl011), it
should be part of the arm specific sub-struct in build_info.
Please also wait a bit for Ian to express his preference for the config
option etc.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-13 8:37 ` Wei Liu
@ 2017-04-19 0:29 ` Stefano Stabellini
2017-04-19 9:17 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 0:29 UTC (permalink / raw)
To: Wei Liu
Cc: Bhupinder Thakur, xen-devel, Julien Grall, Stefano Stabellini,
Ian Jackson
On Thu, 13 Apr 2017, Wei Liu wrote:
> On Thu, Apr 13, 2017 at 01:49:51PM +0530, Bhupinder Thakur wrote:
> > Hi Wei,
> >
> > >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > >> index a612d1f..fe7f795 100644
> > >> --- a/tools/libxl/libxl_types.idl
> > >> +++ b/tools/libxl/libxl_types.idl
> > >> @@ -105,6 +105,7 @@ libxl_console_type = Enumeration("console_type", [
> > >> (0, "UNKNOWN"),
> > >> (1, "SERIAL"),
> > >> (2, "PV"),
> > >> + (3, "VCON"),
> > >> ])
> > >>
> > >
> > > You need to add a LIBXL_HAVE macro to libxl.h.
> >
> > I will add this macro. I am trying to understand the use of these
> > macros. Are the applications which are building against a particular
> > libxl version supposed to check this flag before using this feature?
> >
>
> Yes. Think about a new application that is built against an older
> version of libxl source code that doesn't have this flag. It can use
> this flag to check if such feature is available.
>
> > >
> > >> libxl_disk_format = Enumeration("disk_format", [ @@ -460,6 +461,7
> > >> @@ libxl_domain_build_info = Struct("domain_build_info",[
> > >> ("disable_migrate", libxl_defbool), ("cpuid",
> > >> libxl_cpuid_policy_list), ("blkdev_start", string), +
> > >> ("enable_pl011", libxl_defbool),
> > >>
> > >> ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> > >>
> > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > >> index 358757f..4f4d4e6 100644 --- a/tools/libxl/xl_cmdimpl.c +++
> > >> b/tools/libxl/xl_cmdimpl.c @@ -1433,6 +1433,8 @@ static void
> > >> parse_config_data(const char *config_source, if (!xlu_cfg_get_long
> > >> (config, "maxvcpus", &l, 0)) b_info->max_vcpus = l;
> > >>
> > >> + xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011,
> > >> 0); +
> > >
> > > I'm not very keen on having "pl011" because it is ARM specific.
> > I will rename it to vconsole, which I am using through out the libxl
> > code now.
>
> Before you go away and change your code, we need to discuss this a bit.
>
> Since there is no "console" option in xl, I think using just "console"
> should be fine. And then user can specify "console=pl011" in their
> config file.
I think "console" is confusing an emulated device with xenconsole.
I would use vuart=pl011 to clarify that we are specifying a new
component for emulation.
> There will also need to be changes in the libxl idl struct. If you
> really want to use enable_pl011 (I would suggest to just use pl011), it
> should be part of the arm specific sub-struct in build_info.
>
> Please also wait a bit for Ian to express his preference for the config
> option etc.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-19 0:29 ` Stefano Stabellini
@ 2017-04-19 9:17 ` Bhupinder Thakur
2017-04-19 10:25 ` Wei Liu
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-19 9:17 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Wei Liu, Ian Jackson
Hi,
On 19 April 2017 at 05:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> I think "console" is confusing an emulated device with xenconsole.
>
> I would use vuart=pl011 to clarify that we are specifying a new
> component for emulation.
Throughout the code, I am using "vconsole" or "vcon" as a prefix for
most of the code. Should I replace "vconsole" with "vuart" at all such
places? I did not use "vuart" initially as there was some existing
code in Xen under vuart.c.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-19 9:17 ` Bhupinder Thakur
@ 2017-04-19 10:25 ` Wei Liu
2017-04-19 11:06 ` Julien Grall
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-19 10:25 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson
On Wed, Apr 19, 2017 at 02:47:08PM +0530, Bhupinder Thakur wrote:
> Hi,
>
> On 19 April 2017 at 05:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > I think "console" is confusing an emulated device with xenconsole.
> >
> > I would use vuart=pl011 to clarify that we are specifying a new
> > component for emulation.
>
> Throughout the code, I am using "vconsole" or "vcon" as a prefix for
> most of the code. Should I replace "vconsole" with "vuart" at all such
> places? I did not use "vuart" initially as there was some existing
> code in Xen under vuart.c.
That would be fine by me. But I don't know enough of ARM to make
judgement. Please wait for Stefano or Julien to reply.
Wei.
>
> Regards,
> Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation
2017-04-19 10:25 ` Wei Liu
@ 2017-04-19 11:06 ` Julien Grall
0 siblings, 0 replies; 53+ messages in thread
From: Julien Grall @ 2017-04-19 11:06 UTC (permalink / raw)
To: Wei Liu, Bhupinder Thakur; +Cc: xen-devel, Stefano Stabellini, Ian Jackson
Hi,
On 19/04/17 11:25, Wei Liu wrote:
> On Wed, Apr 19, 2017 at 02:47:08PM +0530, Bhupinder Thakur wrote:
>> Hi,
>>
>> On 19 April 2017 at 05:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> I think "console" is confusing an emulated device with xenconsole.
>>>
>>> I would use vuart=pl011 to clarify that we are specifying a new
>>> component for emulation.
>>
>> Throughout the code, I am using "vconsole" or "vcon" as a prefix for
>> most of the code. Should I replace "vconsole" with "vuart" at all such
>> places? I did not use "vuart" initially as there was some existing
>> code in Xen under vuart.c.
>
> That would be fine by me. But I don't know enough of ARM to make
> judgement. Please wait for Stefano or Julien to reply.
The name "vuart" makes more sense to use for the user point of view.
Regarding vuart.c, this is just a name of a file that is used to expose
a very limited UART to DOM0 that mimic the host UART. In the future we
may want to expose something similar for guest running unmodified OS and
target a specific hardware.
We can rename the file if it is too confusing.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (3 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-12 16:33 ` Wei Liu
2017-04-03 9:44 ` [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore " Bhupinder Thakur
` (5 subsequent siblings)
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
1. Allocate a new pfn and pass on to Xen using a hvm call.
2. Change in xc_hvm_param_deprecated_check () to allow new vpl011 HVM params,
which are reusing some deprecated x86 HVM params.
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/libxc/include/xc_dom.h | 3 +++
tools/libxc/xc_dom_arm.c | 7 ++++++-
tools/libxc/xc_dom_boot.c | 3 +++
tools/libxc/xc_domain.c | 7 +++++++
4 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 608cbc2..c93d98c 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -218,6 +218,9 @@ struct xc_dom_image {
/* Extra SMBIOS structures passed to HVMLOADER */
struct xc_hvm_firmware_module smbios_module;
+
+ /* Virtual console pfn. */
+ xen_pfn_t vconsole_pfn;
};
/* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index a7e839e..157381e 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -26,10 +26,11 @@
#include "xg_private.h"
#include "xc_dom.h"
-#define NR_MAGIC_PAGES 3
+#define NR_MAGIC_PAGES 4
#define CONSOLE_PFN_OFFSET 0
#define XENSTORE_PFN_OFFSET 1
#define MEMACCESS_PFN_OFFSET 2
+#define VCONSOLE_PFN_OFFSET 3
#define LPAE_SHIFT 9
@@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
dom->console_pfn = base + CONSOLE_PFN_OFFSET;
dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+ dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
@@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
dom->xenstore_pfn);
xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
base + MEMACCESS_PFN_OFFSET);
+ xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
+ base + VCONSOLE_PFN_OFFSET);
+
/* allocated by toolstack */
xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
dom->console_evtchn);
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 791041b..ec91b15 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -227,6 +227,9 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
return rc;
+ if ( (rc = clear_page(dom, dom->vconsole_pfn)) != 0 )
+ return rc;
+
/* start info page */
if ( dom->arch_hooks->start_info )
dom->arch_hooks->start_info(dom);
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index fa1daeb..8ac7d4f 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1337,9 +1337,16 @@ static inline int xc_hvm_param_deprecated_check(uint32_t param)
{
switch ( param )
{
+ /*
+ * For Aarch64 the following three parameters are reused and hence this
+ * empty check to avoid returning an error.
+ */
+#if defined (__arm__) || defined(__aarch64__)
+#else
case HVM_PARAM_MEMORY_EVENT_CR0:
case HVM_PARAM_MEMORY_EVENT_CR3:
case HVM_PARAM_MEMORY_EVENT_CR4:
+#endif
case HVM_PARAM_MEMORY_EVENT_INT3:
case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
case HVM_PARAM_MEMORY_EVENT_MSR:
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-03 9:44 ` [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console Bhupinder Thakur
@ 2017-04-12 16:33 ` Wei Liu
2017-04-13 8:37 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-12 16:33 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, Apr 03, 2017 at 03:14:28PM +0530, Bhupinder Thakur wrote:
> 1. Allocate a new pfn and pass on to Xen using a hvm call.
>
> 2. Change in xc_hvm_param_deprecated_check () to allow new vpl011 HVM params,
> which are reusing some deprecated x86 HVM params.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> tools/libxc/include/xc_dom.h | 3 +++
> tools/libxc/xc_dom_arm.c | 7 ++++++-
> tools/libxc/xc_dom_boot.c | 3 +++
> tools/libxc/xc_domain.c | 7 +++++++
> 4 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 608cbc2..c93d98c 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -218,6 +218,9 @@ struct xc_dom_image {
>
> /* Extra SMBIOS structures passed to HVMLOADER */
> struct xc_hvm_firmware_module smbios_module;
> +
> + /* Virtual console pfn. */
> + xen_pfn_t vconsole_pfn;
> };
>
> /* --- pluggable kernel loader ------------------------------------- */
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index a7e839e..157381e 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -26,10 +26,11 @@
> #include "xg_private.h"
> #include "xc_dom.h"
>
> -#define NR_MAGIC_PAGES 3
> +#define NR_MAGIC_PAGES 4
> #define CONSOLE_PFN_OFFSET 0
> #define XENSTORE_PFN_OFFSET 1
> #define MEMACCESS_PFN_OFFSET 2
> +#define VCONSOLE_PFN_OFFSET 3
>
> #define LPAE_SHIFT 9
>
> @@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>
> dom->console_pfn = base + CONSOLE_PFN_OFFSET;
> dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> + dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
>
> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> @@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> dom->xenstore_pfn);
> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
> base + MEMACCESS_PFN_OFFSET);
> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
> + base + VCONSOLE_PFN_OFFSET);
> +
Here is something I don't quite understand (sorry I haven't been
following the conversation closely): if pl011 is emulated, why would the
guest need to know its PFN?
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-12 16:33 ` Wei Liu
@ 2017-04-13 8:37 ` Bhupinder Thakur
2017-04-13 8:53 ` Wei Liu
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-13 8:37 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson
Hi Wei,
>> /* --- pluggable kernel loader ------------------------------------- */
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index a7e839e..157381e 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -26,10 +26,11 @@
>> #include "xg_private.h"
>> #include "xc_dom.h"
>>
>> -#define NR_MAGIC_PAGES 3
>> +#define NR_MAGIC_PAGES 4
>> #define CONSOLE_PFN_OFFSET 0
>> #define XENSTORE_PFN_OFFSET 1
>> #define MEMACCESS_PFN_OFFSET 2
>> +#define VCONSOLE_PFN_OFFSET 3
>>
>> #define LPAE_SHIFT 9
>>
>> @@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>
>> dom->console_pfn = base + CONSOLE_PFN_OFFSET;
>> dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
>> + dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
>>
>> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>> @@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>> dom->xenstore_pfn);
>> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
>> base + MEMACCESS_PFN_OFFSET);
>> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
>> + base + VCONSOLE_PFN_OFFSET);
>> +
>
> Here is something I don't quite understand (sorry I haven't been
> following the conversation closely): if pl011 is emulated, why would the
> guest need to know its PFN?
This PFN is used by Xen to setup a ring-buffer between xenconsole and itself.
Xen reads/writes data from/to this ring buffer when it gets a mmio
read/write request from the guest.
Here using the hvm call, we are passing the PFN to Xen so that it can
setup the ring buffers.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-13 8:37 ` Bhupinder Thakur
@ 2017-04-13 8:53 ` Wei Liu
2017-04-19 0:36 ` Stefano Stabellini
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-13 8:53 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson
On Thu, Apr 13, 2017 at 02:07:54PM +0530, Bhupinder Thakur wrote:
> Hi Wei,
>
>
> >> /* --- pluggable kernel loader ------------------------------------- */
> >> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >> index a7e839e..157381e 100644
> >> --- a/tools/libxc/xc_dom_arm.c
> >> +++ b/tools/libxc/xc_dom_arm.c
> >> @@ -26,10 +26,11 @@
> >> #include "xg_private.h"
> >> #include "xc_dom.h"
> >>
> >> -#define NR_MAGIC_PAGES 3
> >> +#define NR_MAGIC_PAGES 4
> >> #define CONSOLE_PFN_OFFSET 0
> >> #define XENSTORE_PFN_OFFSET 1
> >> #define MEMACCESS_PFN_OFFSET 2
> >> +#define VCONSOLE_PFN_OFFSET 3
> >>
> >> #define LPAE_SHIFT 9
> >>
> >> @@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> >>
> >> dom->console_pfn = base + CONSOLE_PFN_OFFSET;
> >> dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> >> + dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
> >>
> >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
> >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> >> @@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> >> dom->xenstore_pfn);
> >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
> >> base + MEMACCESS_PFN_OFFSET);
> >> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
> >> + base + VCONSOLE_PFN_OFFSET);
> >> +
> >
> > Here is something I don't quite understand (sorry I haven't been
> > following the conversation closely): if pl011 is emulated, why would the
> > guest need to know its PFN?
>
> This PFN is used by Xen to setup a ring-buffer between xenconsole and itself.
> Xen reads/writes data from/to this ring buffer when it gets a mmio
> read/write request from the guest.
>
What I was getting at was "does the *guest* need to know the PFN"? The
hypervisor and xenconsole daemon / client aren't the guest. Does the
guest need to know the exact PFN in order to setup MMIO?
Ultimately this is going to be decided by ARM maintainers. I'm just
curious about why it is done like this.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-13 8:53 ` Wei Liu
@ 2017-04-19 0:36 ` Stefano Stabellini
2017-04-19 10:28 ` Wei Liu
0 siblings, 1 reply; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 0:36 UTC (permalink / raw)
To: Wei Liu
Cc: Bhupinder Thakur, xen-devel, Julien Grall, Stefano Stabellini,
Ian Jackson
On Thu, 13 Apr 2017, Wei Liu wrote:
> On Thu, Apr 13, 2017 at 02:07:54PM +0530, Bhupinder Thakur wrote:
> > Hi Wei,
> >
> >
> > >> /* --- pluggable kernel loader ------------------------------------- */
> > >> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> > >> index a7e839e..157381e 100644
> > >> --- a/tools/libxc/xc_dom_arm.c
> > >> +++ b/tools/libxc/xc_dom_arm.c
> > >> @@ -26,10 +26,11 @@
> > >> #include "xg_private.h"
> > >> #include "xc_dom.h"
> > >>
> > >> -#define NR_MAGIC_PAGES 3
> > >> +#define NR_MAGIC_PAGES 4
> > >> #define CONSOLE_PFN_OFFSET 0
> > >> #define XENSTORE_PFN_OFFSET 1
> > >> #define MEMACCESS_PFN_OFFSET 2
> > >> +#define VCONSOLE_PFN_OFFSET 3
> > >>
> > >> #define LPAE_SHIFT 9
> > >>
> > >> @@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> > >>
> > >> dom->console_pfn = base + CONSOLE_PFN_OFFSET;
> > >> dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> > >> + dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
> > >>
> > >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
> > >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> > >> @@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> > >> dom->xenstore_pfn);
> > >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
> > >> base + MEMACCESS_PFN_OFFSET);
> > >> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
> > >> + base + VCONSOLE_PFN_OFFSET);
> > >> +
> > >
> > > Here is something I don't quite understand (sorry I haven't been
> > > following the conversation closely): if pl011 is emulated, why would the
> > > guest need to know its PFN?
> >
> > This PFN is used by Xen to setup a ring-buffer between xenconsole and itself.
> > Xen reads/writes data from/to this ring buffer when it gets a mmio
> > read/write request from the guest.
> >
>
> What I was getting at was "does the *guest* need to know the PFN"? The
> hypervisor and xenconsole daemon / client aren't the guest. Does the
> guest need to know the exact PFN in order to setup MMIO?
>
> Ultimately this is going to be decided by ARM maintainers. I'm just
> curious about why it is done like this.
hvm_params are commonly used to pass parameters from Xen or from libxl
to guests. In this case, they are used to pass parameters from the
toolstack to Xen.
The guest does not need to know the pfn, and in fact it cannot: Xen
refuses to return the value of HVM_PARAM_VCONSOLE_PFN to guests (see
patch #2).
Honestly, I don't particularly care about how the pfn is passed from
libxc to Xen. hvm_param is an option, or we could introduce a new
domctl.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-19 0:36 ` Stefano Stabellini
@ 2017-04-19 10:28 ` Wei Liu
2017-04-19 11:01 ` Julien Grall
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-19 10:28 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Bhupinder Thakur, xen-devel, Julien Grall, Wei Liu, Ian Jackson
On Tue, Apr 18, 2017 at 05:36:41PM -0700, Stefano Stabellini wrote:
> On Thu, 13 Apr 2017, Wei Liu wrote:
> > On Thu, Apr 13, 2017 at 02:07:54PM +0530, Bhupinder Thakur wrote:
> > > Hi Wei,
> > >
> > >
> > > >> /* --- pluggable kernel loader ------------------------------------- */
> > > >> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> > > >> index a7e839e..157381e 100644
> > > >> --- a/tools/libxc/xc_dom_arm.c
> > > >> +++ b/tools/libxc/xc_dom_arm.c
> > > >> @@ -26,10 +26,11 @@
> > > >> #include "xg_private.h"
> > > >> #include "xc_dom.h"
> > > >>
> > > >> -#define NR_MAGIC_PAGES 3
> > > >> +#define NR_MAGIC_PAGES 4
> > > >> #define CONSOLE_PFN_OFFSET 0
> > > >> #define XENSTORE_PFN_OFFSET 1
> > > >> #define MEMACCESS_PFN_OFFSET 2
> > > >> +#define VCONSOLE_PFN_OFFSET 3
> > > >>
> > > >> #define LPAE_SHIFT 9
> > > >>
> > > >> @@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> > > >>
> > > >> dom->console_pfn = base + CONSOLE_PFN_OFFSET;
> > > >> dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> > > >> + dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
> > > >>
> > > >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
> > > >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> > > >> @@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> > > >> dom->xenstore_pfn);
> > > >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
> > > >> base + MEMACCESS_PFN_OFFSET);
> > > >> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
> > > >> + base + VCONSOLE_PFN_OFFSET);
> > > >> +
> > > >
> > > > Here is something I don't quite understand (sorry I haven't been
> > > > following the conversation closely): if pl011 is emulated, why would the
> > > > guest need to know its PFN?
> > >
> > > This PFN is used by Xen to setup a ring-buffer between xenconsole and itself.
> > > Xen reads/writes data from/to this ring buffer when it gets a mmio
> > > read/write request from the guest.
> > >
> >
> > What I was getting at was "does the *guest* need to know the PFN"? The
> > hypervisor and xenconsole daemon / client aren't the guest. Does the
> > guest need to know the exact PFN in order to setup MMIO?
> >
> > Ultimately this is going to be decided by ARM maintainers. I'm just
> > curious about why it is done like this.
>
> hvm_params are commonly used to pass parameters from Xen or from libxl
> to guests. In this case, they are used to pass parameters from the
> toolstack to Xen.
>
> The guest does not need to know the pfn, and in fact it cannot: Xen
> refuses to return the value of HVM_PARAM_VCONSOLE_PFN to guests (see
> patch #2).
>
Ah, I missed the new restriction in #2.
> Honestly, I don't particularly care about how the pfn is passed from
> libxc to Xen. hvm_param is an option, or we could introduce a new
> domctl.
No opinion from me either. I think using HVM params is fine.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-19 10:28 ` Wei Liu
@ 2017-04-19 11:01 ` Julien Grall
2017-04-19 13:05 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Julien Grall @ 2017-04-19 11:01 UTC (permalink / raw)
To: Wei Liu, Stefano Stabellini; +Cc: Bhupinder Thakur, xen-devel, Ian Jackson
Hi,
On 19/04/17 11:28, Wei Liu wrote:
> On Tue, Apr 18, 2017 at 05:36:41PM -0700, Stefano Stabellini wrote:
>> On Thu, 13 Apr 2017, Wei Liu wrote:
>>> On Thu, Apr 13, 2017 at 02:07:54PM +0530, Bhupinder Thakur wrote:
>>>> Hi Wei,
>>>>
>>>>
>>>>>> /* --- pluggable kernel loader ------------------------------------- */
>>>>>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>>>>>> index a7e839e..157381e 100644
>>>>>> --- a/tools/libxc/xc_dom_arm.c
>>>>>> +++ b/tools/libxc/xc_dom_arm.c
>>>>>> @@ -26,10 +26,11 @@
>>>>>> #include "xg_private.h"
>>>>>> #include "xc_dom.h"
>>>>>>
>>>>>> -#define NR_MAGIC_PAGES 3
>>>>>> +#define NR_MAGIC_PAGES 4
>>>>>> #define CONSOLE_PFN_OFFSET 0
>>>>>> #define XENSTORE_PFN_OFFSET 1
>>>>>> #define MEMACCESS_PFN_OFFSET 2
>>>>>> +#define VCONSOLE_PFN_OFFSET 3
>>>>>>
>>>>>> #define LPAE_SHIFT 9
>>>>>>
>>>>>> @@ -85,6 +86,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>>>>>
>>>>>> dom->console_pfn = base + CONSOLE_PFN_OFFSET;
>>>>>> dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
>>>>>> + dom->vconsole_pfn = base + VCONSOLE_PFN_OFFSET;
>>>>>>
>>>>>> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>>>>>> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>>>>>> @@ -95,6 +97,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>>>>> dom->xenstore_pfn);
>>>>>> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
>>>>>> base + MEMACCESS_PFN_OFFSET);
>>>>>> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VCONSOLE_PFN,
>>>>>> + base + VCONSOLE_PFN_OFFSET);
>>>>>> +
>>>>>
>>>>> Here is something I don't quite understand (sorry I haven't been
>>>>> following the conversation closely): if pl011 is emulated, why would the
>>>>> guest need to know its PFN?
>>>>
>>>> This PFN is used by Xen to setup a ring-buffer between xenconsole and itself.
>>>> Xen reads/writes data from/to this ring buffer when it gets a mmio
>>>> read/write request from the guest.
>>>>
>>>
>>> What I was getting at was "does the *guest* need to know the PFN"? The
>>> hypervisor and xenconsole daemon / client aren't the guest. Does the
>>> guest need to know the exact PFN in order to setup MMIO?
>>>
>>> Ultimately this is going to be decided by ARM maintainers. I'm just
>>> curious about why it is done like this.
>>
>> hvm_params are commonly used to pass parameters from Xen or from libxl
>> to guests. In this case, they are used to pass parameters from the
>> toolstack to Xen.
>>
>> The guest does not need to know the pfn, and in fact it cannot: Xen
>> refuses to return the value of HVM_PARAM_VCONSOLE_PFN to guests (see
>> patch #2).
>>
>
> Ah, I missed the new restriction in #2.
>
>> Honestly, I don't particularly care about how the pfn is passed from
>> libxc to Xen. hvm_param is an option, or we could introduce a new
>> domctl.
>
> No opinion from me either. I think using HVM params is fine.
There were some concerns on the first version about using HVM params
because this is tying the virtual UART to HVM and only allow us to have
one virtual console.
HVM params are part of the stable ABI, so this would restrict ourselves
to future extension.
It was suggested to look at the DOMCTL way, I would have expected to
some investigation and a summary in the cover letter. So why do we keep
HVM PARAM?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-19 11:01 ` Julien Grall
@ 2017-04-19 13:05 ` Bhupinder Thakur
2017-04-19 13:35 ` Julien Grall
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-19 13:05 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Wei Liu, Ian Jackson
Hi,
On 19 April 2017 at 16:31, Julien Grall <julien.grall@arm.com> wrote:
> There were some concerns on the first version about using HVM params because
> this is tying the virtual UART to HVM and only allow us to have one virtual
> console.
>
> HVM params are part of the stable ABI, so this would restrict ourselves to
> future extension.
>
> It was suggested to look at the DOMCTL way, I would have expected to some
> investigation and a summary in the cover letter. So why do we keep HVM
> PARAM?
It is more flexible to use DOMCTL interface in terms of supporting
multiple vUARTs in future without changing the interface. This DOMCTL
interface cannot be called from xenconsoled though because xenconsoled
does not know the PFN to be passed to Xen. It is ok to call this
DOMCTL interface from the toolstack since it knows the PFN and can
pass it on to Xen.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console
2017-04-19 13:05 ` Bhupinder Thakur
@ 2017-04-19 13:35 ` Julien Grall
0 siblings, 0 replies; 53+ messages in thread
From: Julien Grall @ 2017-04-19 13:35 UTC (permalink / raw)
To: Bhupinder Thakur; +Cc: xen-devel, Stefano Stabellini, Wei Liu, Ian Jackson
On 19/04/17 14:05, Bhupinder Thakur wrote:
> Hi,
Hi Bhupinder,
> On 19 April 2017 at 16:31, Julien Grall <julien.grall@arm.com> wrote:
>> There were some concerns on the first version about using HVM params because
>> this is tying the virtual UART to HVM and only allow us to have one virtual
>> console.
>>
>> HVM params are part of the stable ABI, so this would restrict ourselves to
>> future extension.
>>
>> It was suggested to look at the DOMCTL way, I would have expected to some
>> investigation and a summary in the cover letter. So why do we keep HVM
>> PARAM?
>
> It is more flexible to use DOMCTL interface in terms of supporting
> multiple vUARTs in future without changing the interface. This DOMCTL
> interface cannot be called from xenconsoled though because xenconsoled
> does not know the PFN to be passed to Xen. It is ok to call this
> DOMCTL interface from the toolstack since it knows the PFN and can
> pass it on to Xen.
I am not sure to understand what you mean. The DOMCTL could be used to
set the PFN by the toolstack and then retrieved by the console.
You could also use xenstore to pass the PFN. So what is the problem?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore for the virtual console
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (4 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 05/10] xen/arm: vpl011: Allocate a new PFN in the toolstack for the virtual console Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-12 16:32 ` Wei Liu
2017-04-19 18:58 ` Stefano Stabellini
2017-04-03 9:44 ` [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole Bhupinder Thakur
` (4 subsequent siblings)
10 siblings, 2 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
Add two new parameters to the xen store:
- newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled
- a new event channel read from Xen using a hvm call to be used by xenconsoled
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/libxl/libxl.c | 10 ++++++++++
tools/libxl/libxl_dom.c | 13 ++++++++++++-
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d400fa2..5fa1e41 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1791,6 +1791,9 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
case LIBXL_CONSOLE_TYPE_SERIAL:
cons_type_s = "serial";
break;
+ case LIBXL_CONSOLE_TYPE_VCON:
+ cons_type_s = "vcon";
+ break;
default:
goto out;
}
@@ -3159,6 +3162,13 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
flexarray_append(ro_front, "ring-ref");
flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
+ if (state->vconsole_enabled)
+ {
+ flexarray_append(ro_front, "vcon-port");
+ flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vconsole_port));
+ flexarray_append(ro_front, "vcon-ring-ref");
+ flexarray_append(ro_front, GCSPRINTF("%lu", state->vconsole_mfn));
+ }
} else {
flexarray_append(front, "state");
flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d519c8d..8df1e10 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -302,7 +302,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
libxl_ctx *ctx = libxl__gc_owner(gc);
char *xs_domid, *con_domid;
int rc;
- uint64_t size;
+ uint64_t size, val;
if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
LOG(ERROR, "Couldn't set max vcpu count");
@@ -432,6 +432,16 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
+ state->vconsole_port = -1;
+
+ if (state->vconsole_enabled)
+ {
+ rc = xc_hvm_param_get(ctx->xch, domid, HVM_PARAM_VCONSOLE_EVTCHN,
+ &val);
+ if ( !rc )
+ state->vconsole_port = val;
+ }
+
if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
hvm_set_conf_params(ctx->xch, domid, info);
#if defined(__i386__) || defined(__x86_64__)
@@ -771,6 +781,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
if (xc_dom_feature_translated(dom)) {
state->console_mfn = dom->console_pfn;
state->store_mfn = dom->xenstore_pfn;
+ state->vconsole_mfn = dom->vconsole_pfn;
} else {
state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore for the virtual console
2017-04-03 9:44 ` [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore " Bhupinder Thakur
@ 2017-04-12 16:32 ` Wei Liu
2017-04-25 10:18 ` Bhupinder Thakur
2017-04-19 18:58 ` Stefano Stabellini
1 sibling, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-12 16:32 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, Apr 03, 2017 at 03:14:29PM +0530, Bhupinder Thakur wrote:
> Add two new parameters to the xen store:
> - newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled
> - a new event channel read from Xen using a hvm call to be used by xenconsoled
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> tools/libxl/libxl.c | 10 ++++++++++
> tools/libxl/libxl_dom.c | 13 ++++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d400fa2..5fa1e41 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1791,6 +1791,9 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> case LIBXL_CONSOLE_TYPE_SERIAL:
> cons_type_s = "serial";
> break;
> + case LIBXL_CONSOLE_TYPE_VCON:
> + cons_type_s = "vcon";
> + break;
> default:
> goto out;
> }
> @@ -3159,6 +3162,13 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
> flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
> flexarray_append(ro_front, "ring-ref");
> flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
> + if (state->vconsole_enabled)
> + {
> + flexarray_append(ro_front, "vcon-port");
> + flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vconsole_port));
> + flexarray_append(ro_front, "vcon-ring-ref");
> + flexarray_append(ro_front, GCSPRINTF("%lu", state->vconsole_mfn));
> + }
Please try to document these new xenstore paths. See
docs/misc/xenstore-paths.md.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore for the virtual console
2017-04-12 16:32 ` Wei Liu
@ 2017-04-25 10:18 ` Bhupinder Thakur
2017-04-25 11:55 ` Wei Liu
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-25 10:18 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson
Hi Wei,
>> }
>> @@ -3159,6 +3162,13 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>> flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
>> flexarray_append(ro_front, "ring-ref");
>> flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
>> + if (state->vconsole_enabled)
>> + {
>> + flexarray_append(ro_front, "vcon-port");
>> + flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vconsole_port));
>> + flexarray_append(ro_front, "vcon-ring-ref");
>> + flexarray_append(ro_front, GCSPRINTF("%lu", state->vconsole_mfn));
>> + }
>
> Please try to document these new xenstore paths. See
> docs/misc/xenstore-paths.md.
I will add the vuart console details in console.txt which is
referenced in xenstore-paths.md.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore for the virtual console
2017-04-25 10:18 ` Bhupinder Thakur
@ 2017-04-25 11:55 ` Wei Liu
0 siblings, 0 replies; 53+ messages in thread
From: Wei Liu @ 2017-04-25 11:55 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson
On Tue, Apr 25, 2017 at 03:48:47PM +0530, Bhupinder Thakur wrote:
> Hi Wei,
>
>
> >> }
> >> @@ -3159,6 +3162,13 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
> >> flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
> >> flexarray_append(ro_front, "ring-ref");
> >> flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
> >> + if (state->vconsole_enabled)
> >> + {
> >> + flexarray_append(ro_front, "vcon-port");
> >> + flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vconsole_port));
> >> + flexarray_append(ro_front, "vcon-ring-ref");
> >> + flexarray_append(ro_front, GCSPRINTF("%lu", state->vconsole_mfn));
> >> + }
> >
> > Please try to document these new xenstore paths. See
> > docs/misc/xenstore-paths.md.
>
> I will add the vuart console details in console.txt which is
> referenced in xenstore-paths.md.
>
Yes that's good.
> Regards,
> Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore for the virtual console
2017-04-03 9:44 ` [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore " Bhupinder Thakur
2017-04-12 16:32 ` Wei Liu
@ 2017-04-19 18:58 ` Stefano Stabellini
1 sibling, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 18:58 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> Add two new parameters to the xen store:
> - newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled
> - a new event channel read from Xen using a hvm call to be used by xenconsoled
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
please use vuart instead of vcon
> ---
> tools/libxl/libxl.c | 10 ++++++++++
> tools/libxl/libxl_dom.c | 13 ++++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d400fa2..5fa1e41 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1791,6 +1791,9 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> case LIBXL_CONSOLE_TYPE_SERIAL:
> cons_type_s = "serial";
> break;
> + case LIBXL_CONSOLE_TYPE_VCON:
> + cons_type_s = "vcon";
> + break;
> default:
> goto out;
> }
> @@ -3159,6 +3162,13 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
> flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
> flexarray_append(ro_front, "ring-ref");
> flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
> + if (state->vconsole_enabled)
> + {
> + flexarray_append(ro_front, "vcon-port");
> + flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vconsole_port));
> + flexarray_append(ro_front, "vcon-ring-ref");
> + flexarray_append(ro_front, GCSPRINTF("%lu", state->vconsole_mfn));
it makes more sense to add a new sub-directory such as:
ro_front/vuart/port
ro_front/vuart/ring-ref
Even better, with the idea of supporting more than one in the future:
ro_front/vuart/0/port
ro_front/vuart/0/ring-ref
> + }
> } else {
> flexarray_append(front, "state");
> flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index d519c8d..8df1e10 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -302,7 +302,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> libxl_ctx *ctx = libxl__gc_owner(gc);
> char *xs_domid, *con_domid;
> int rc;
> - uint64_t size;
> + uint64_t size, val;
>
> if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
> LOG(ERROR, "Couldn't set max vcpu count");
> @@ -432,6 +432,16 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
> state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
>
> + state->vconsole_port = -1;
> +
> + if (state->vconsole_enabled)
> + {
> + rc = xc_hvm_param_get(ctx->xch, domid, HVM_PARAM_VCONSOLE_EVTCHN,
> + &val);
> + if ( !rc )
> + state->vconsole_port = val;
> + }
> +
> if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
> hvm_set_conf_params(ctx->xch, domid, info);
> #if defined(__i386__) || defined(__x86_64__)
> @@ -771,6 +781,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> if (xc_dom_feature_translated(dom)) {
> state->console_mfn = dom->console_pfn;
> state->store_mfn = dom->xenstore_pfn;
> + state->vconsole_mfn = dom->vconsole_pfn;
> } else {
> state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
> state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (5 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 06/10] xen/arm: vpl011: Add new parameters to xenstore " Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-12 16:33 ` Wei Liu
2017-04-19 19:09 ` Stefano Stabellini
2017-04-03 9:44 ` [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles Bhupinder Thakur
` (3 subsequent siblings)
10 siblings, 2 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
Modify the domain structure to to make console specific fields as an array indexed
by the console type. Two console types are defined - PV and VCON.
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/console/daemon/io.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..0cd1fee 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -61,6 +61,10 @@
/* Duration of each time period in ms */
#define RATE_LIMIT_PERIOD 200
+#define MAX_CONSOLE 2
+#define CONSOLE_TYPE_PV 0
+#define CONSOLE_TYPE_VCON 1
+
extern int log_reload;
extern int log_guest;
extern int log_hv;
@@ -91,23 +95,25 @@ struct buffer {
struct domain {
int domid;
- int master_fd;
- int master_pollfd_idx;
- int slave_fd;
- int log_fd;
+ int master_fd[MAX_CONSOLE];
+ int master_pollfd_idx[MAX_CONSOLE];
+ int slave_fd[MAX_CONSOLE];
+ int log_fd[MAX_CONSOLE];
bool is_dead;
unsigned last_seen;
- struct buffer buffer;
+ struct buffer buffer[MAX_CONSOLE];
struct domain *next;
char *conspath;
- int ring_ref;
- xenevtchn_port_or_error_t local_port;
- xenevtchn_port_or_error_t remote_port;
+ int ring_ref[MAX_CONSOLE];
+ xenevtchn_port_or_error_t local_port[MAX_CONSOLE];
+ xenevtchn_port_or_error_t remote_port[MAX_CONSOLE];
xenevtchn_handle *xce_handle;
int xce_pollfd_idx;
- struct xencons_interface *interface;
+ struct xencons_interface *interface[MAX_CONSOLE];
int event_count;
long long next_period;
+ int console_data_pending;
+ bool vcon_enabled;
};
static struct domain *dom_head;
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole
2017-04-03 9:44 ` [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole Bhupinder Thakur
@ 2017-04-12 16:33 ` Wei Liu
2017-04-13 9:49 ` Bhupinder Thakur
2017-04-19 19:09 ` Stefano Stabellini
1 sibling, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-12 16:33 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, Apr 03, 2017 at 03:14:30PM +0530, Bhupinder Thakur wrote:
> Modify the domain structure to to make console specific fields as an array indexed
> by the console type. Two console types are defined - PV and VCON.
Why? Can you have both for an ARM guest?
Also this patch alone is going to break xencosnole. Please make sure the
whole series is bisectable.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole
2017-04-12 16:33 ` Wei Liu
@ 2017-04-13 9:49 ` Bhupinder Thakur
0 siblings, 0 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-13 9:49 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson
Hi Wei,
On 12 April 2017 at 22:03, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Apr 03, 2017 at 03:14:30PM +0530, Bhupinder Thakur wrote:
>> Modify the domain structure to to make console specific fields as an array indexed
>> by the console type. Two console types are defined - PV and VCON.
>
> Why? Can you have both for an ARM guest?
>
Ideally, user could specify multiple consoles for a domU guest.
> Also this patch alone is going to break xencosnole. Please make sure the
> whole series is bisectable.
>
I will review the patch series to see if there are any other
bisectability issues.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole
2017-04-03 9:44 ` [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole Bhupinder Thakur
2017-04-12 16:33 ` Wei Liu
@ 2017-04-19 19:09 ` Stefano Stabellini
1 sibling, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 19:09 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> Modify the domain structure to to make console specific fields as an array indexed
> by the console type. Two console types are defined - PV and VCON.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> tools/console/daemon/io.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e6a886..0cd1fee 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -61,6 +61,10 @@
> /* Duration of each time period in ms */
> #define RATE_LIMIT_PERIOD 200
>
> +#define MAX_CONSOLE 2
> +#define CONSOLE_TYPE_PV 0
> +#define CONSOLE_TYPE_VCON 1
> +
> extern int log_reload;
> extern int log_guest;
> extern int log_hv;
> @@ -91,23 +95,25 @@ struct buffer {
>
> struct domain {
> int domid;
> - int master_fd;
> - int master_pollfd_idx;
> - int slave_fd;
> - int log_fd;
> + int master_fd[MAX_CONSOLE];
> + int master_pollfd_idx[MAX_CONSOLE];
> + int slave_fd[MAX_CONSOLE];
> + int log_fd[MAX_CONSOLE];
> bool is_dead;
> unsigned last_seen;
> - struct buffer buffer;
> + struct buffer buffer[MAX_CONSOLE];
> struct domain *next;
> char *conspath;
> - int ring_ref;
> - xenevtchn_port_or_error_t local_port;
> - xenevtchn_port_or_error_t remote_port;
> + int ring_ref[MAX_CONSOLE];
> + xenevtchn_port_or_error_t local_port[MAX_CONSOLE];
> + xenevtchn_port_or_error_t remote_port[MAX_CONSOLE];
> xenevtchn_handle *xce_handle;
> int xce_pollfd_idx;
> - struct xencons_interface *interface;
> + struct xencons_interface *interface[MAX_CONSOLE];
> int event_count;
> long long next_period;
> + int console_data_pending;
> + bool vcon_enabled;
I think you need to gather together all the info of one console into a
single struct, maybe called struct console. Then you'll have an array of
MAX_CONSOLE struct consoles for each domain. Something like:
struct console {
int master_fd;
int master_pollfd_idx;
int slave_fd;
int log_fd;
struct buffer buffer;
int ring_ref;
xenevtchn_port_or_error_t local_port;
xenevtchn_port_or_error_t remote_port;
struct xencons_interface *interface;
}
At that point, you'll be able to generalize all functions in this file,
like buffer_append and handle_ring_read, to take a struct console* as
argument instead of a struct domain*, and they will work seamlessly on
the old pv console and the new vuart console.
The for loop in handle_io can figure out which struct console* to pass
as parameter.
The resulting patch will be much nicer.
> };
>
> static struct domain *dom_head;
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (6 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 07/10] xen/arm: vpl011: Add a new console type to domain structure in xenconsole Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-12 16:33 ` Wei Liu
2017-04-03 9:44 ` [PATCH 09/10] xen/arm: vpl011: Add new virtual console to xenconsole client Bhupinder Thakur
` (2 subsequent siblings)
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
Xenconsole supports only PV console currently. To get access to emulated pl011
uart another backend console is required.
This patch modifies different data structures and APIs used
in xenconsole to support two console types: PV and VCON (virtual console).
Change summary:
1. Modify the domain structure to support two console types: PV and a
virtual console (VCON).
2. Modify different APIs such as buffer_append() to take a new parameter
console_type as input and operate on the data structures indexed by
the console_type.
3. Modfications in domain_create_ring():
- Bind to the vpl011 event channel obtained from the xen store as a
new
parameter
- Map the PFN to its address space to be used as IN/OUT ring
buffers.
It obtains the PFN from the xen store as a new parameter
4. Modifications in handle_ring_read() to handle both PV and VCON
events.
5. Add a new log_file for VCON console logs.
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/console/daemon/io.c | 508 ++++++++++++++++++++++++++++++++--------------
1 file changed, 356 insertions(+), 152 deletions(-)
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 0cd1fee..b9be5a5 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -164,11 +164,39 @@ static int write_with_timestamp(int fd, const char *data, size_t sz,
return 0;
}
-static void buffer_append(struct domain *dom)
+/*
+ * This function checks if the data is allowed to be buffered for that console.
+ * If not then it marks it pending for later receiving.
+ */
+static bool buffer_available(struct domain *dom, int console_type)
+{
+ if (discard_overflowed_data ||
+ !dom->buffer[console_type].max_capacity ||
+ dom->buffer[console_type].size < dom->buffer[console_type].max_capacity)
+ {
+ dom->console_data_pending &= ~(1<<console_type);
+ return true;
+ }
+ else
+ {
+ dom->console_data_pending |= (1<<console_type);
+ return false;
+ }
+}
+
+static void buffer_append(struct domain *dom, int console_type)
{
- struct buffer *buffer = &dom->buffer;
+ struct buffer *buffer = &dom->buffer[console_type];
+ struct xencons_interface *intf=dom->interface[console_type];
+ xenevtchn_port_or_error_t port=dom->local_port[console_type];
+
XENCONS_RING_IDX cons, prod, size;
- struct xencons_interface *intf = dom->interface;
+
+ /*
+ * check if more data is allowed to be buffered
+ */
+ if (!buffer_available(dom, console_type))
+ return;
cons = intf->out_cons;
prod = intf->out_prod;
@@ -193,22 +221,22 @@ static void buffer_append(struct domain *dom)
xen_mb();
intf->out_cons = cons;
- xenevtchn_notify(dom->xce_handle, dom->local_port);
+ xenevtchn_notify(dom->xce_handle, port);
/* Get the data to the logfile as early as possible because if
* no one is listening on the console pty then it will fill up
* and handle_tty_write will stop being called.
*/
- if (dom->log_fd != -1) {
+ if (dom->log_fd[console_type] != -1) {
int logret;
if (log_time_guest) {
logret = write_with_timestamp(
- dom->log_fd,
+ dom->log_fd[console_type],
buffer->data + buffer->size - size,
size, &log_time_guest_needts);
} else {
logret = write_all(
- dom->log_fd,
+ dom->log_fd[console_type],
buffer->data + buffer->size - size,
size);
}
@@ -296,12 +324,13 @@ static int create_hv_log(void)
return fd;
}
-static int create_domain_log(struct domain *dom)
+static int create_domain_log(struct domain *dom, int console_type)
{
char logfile[PATH_MAX];
char *namepath, *data, *s;
int fd;
unsigned int len;
+ char *console_name[]={"pv", "vcon"};
namepath = xs_get_domain_path(xs, dom->domid);
s = realloc(namepath, strlen(namepath) + 6);
@@ -320,7 +349,7 @@ static int create_domain_log(struct domain *dom)
return -1;
}
- snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
+ snprintf(logfile, PATH_MAX-1, "%s/guest-%s-%s.log", log_dir, console_name[console_type], data);
free(data);
logfile[PATH_MAX-1] = '\0';
@@ -344,14 +373,24 @@ static int create_domain_log(struct domain *dom)
static void domain_close_tty(struct domain *dom)
{
- if (dom->master_fd != -1) {
- close(dom->master_fd);
- dom->master_fd = -1;
+ if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
+ close(dom->master_fd[CONSOLE_TYPE_PV]);
+ dom->master_fd[CONSOLE_TYPE_PV] = -1;
+ }
+
+ if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
+ close(dom->slave_fd[CONSOLE_TYPE_PV]);
+ dom->slave_fd[CONSOLE_TYPE_PV] = -1;
+ }
+
+ if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
+ close(dom->master_fd[CONSOLE_TYPE_VCON]);
+ dom->master_fd[CONSOLE_TYPE_VCON] = -1;
}
- if (dom->slave_fd != -1) {
- close(dom->slave_fd);
- dom->slave_fd = -1;
+ if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
+ close(dom->slave_fd[CONSOLE_TYPE_VCON]);
+ dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
}
}
@@ -424,63 +463,86 @@ static int domain_create_tty(struct domain *dom)
char *data;
unsigned int len;
struct termios term;
+ int console_type=0;
- assert(dom->slave_fd == -1);
- assert(dom->master_fd == -1);
+ assert(dom->master_fd[CONSOLE_TYPE_PV] == -1);
+ assert(dom->slave_fd[CONSOLE_TYPE_PV] == -1);
+ assert(dom->master_fd[CONSOLE_TYPE_VCON] == -1);
+ assert(dom->slave_fd[CONSOLE_TYPE_VCON] == -1);
- if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
- err = errno;
- dolog(LOG_ERR, "Failed to create tty for domain-%d "
- "(errno = %i, %s)",
- dom->domid, err, strerror(err));
- return 0;
- }
+ /*
+ * Open pty for both PV and vcon consoles.
+ */
+ for (console_type=0; console_type<MAX_CONSOLE; console_type++)
+ {
+ if (openpty(&dom->master_fd[console_type], &dom->slave_fd[console_type], NULL, NULL, NULL) < 0) {
+ err = errno;
+ dolog(LOG_ERR, "Failed to create tty for domain-%d "
+ "(errno = %i, %s)",
+ dom->domid, err, strerror(err));
+ return 0;
+ }
- if (tcgetattr(dom->slave_fd, &term) < 0) {
- err = errno;
- dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
- "(errno = %i, %s)",
- dom->domid, err, strerror(err));
- goto out;
- }
- cfmakeraw(&term);
- if (tcsetattr(dom->slave_fd, TCSANOW, &term) < 0) {
- err = errno;
- dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
- "(errno = %i, %s)",
- dom->domid, err, strerror(err));
- goto out;
- }
+ if (tcgetattr(dom->slave_fd[console_type], &term) < 0) {
+ err = errno;
+ dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
+ "(errno = %i, %s)",
+ dom->domid, err, strerror(err));
+ goto out;
+ }
+ cfmakeraw(&term);
+ if (tcsetattr(dom->slave_fd[console_type], TCSANOW, &term) < 0) {
+ err = errno;
+ dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
+ "(errno = %i, %s)",
+ dom->domid, err, strerror(err));
+ goto out;
+ }
- if ((slave = ptsname(dom->master_fd)) == NULL) {
- err = errno;
- dolog(LOG_ERR, "Failed to get slave name for domain-%d "
- "(errno = %i, %s)",
- dom->domid, err, strerror(err));
- goto out;
- }
+ if ((slave = ptsname(dom->master_fd[console_type])) == NULL) {
+ err = errno;
+ dolog(LOG_ERR, "Failed to get slave name for domain-%d "
+ "(errno = %i, %s)",
+ dom->domid, err, strerror(err));
+ goto out;
+ }
- success = asprintf(&path, "%s/limit", dom->conspath) !=
- -1;
- if (!success)
- goto out;
- data = xs_read(xs, XBT_NULL, path, &len);
- if (data) {
- dom->buffer.max_capacity = strtoul(data, 0, 0);
- free(data);
+ /*
+ * Initialize the console buffer capacity.
+ */
+ success = asprintf(&path, "%s/limit", dom->conspath) !=
+ -1;
+ if (!success)
+ goto out;
+ data = xs_read(xs, XBT_NULL, path, &len);
+ if (data) {
+ dom->buffer[console_type].max_capacity = strtoul(data, 0, 0);
+ free(data);
+ }
+ free(path);
+
+ /*
+ * Write console slave name to xen store.
+ */
+ if (console_type == CONSOLE_TYPE_VCON)
+ success = (asprintf(&path, "%s/vtty", dom->conspath) != -1);
+ else
+ success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
+
+ if (!success)
+ goto out;
+ success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
+ free(path);
+
+ if (fcntl(dom->master_fd[console_type], F_SETFL, O_NONBLOCK) == -1)
+ goto out;
+
+ if (!dom->vcon_enabled)
+ break;
}
- free(path);
- success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
if (!success)
goto out;
- success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
- free(path);
- if (!success)
- goto out;
-
- if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
- goto out;
return 1;
out:
@@ -523,21 +585,53 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
return ret;
}
-static void domain_unmap_interface(struct domain *dom)
+static void domain_unmap_interface(struct domain *dom, int console_type)
{
- if (dom->interface == NULL)
+ if (dom->interface[console_type] == NULL)
return;
- if (xgt_handle && dom->ring_ref == -1)
- xengnttab_unmap(xgt_handle, dom->interface, 1);
+ if (xgt_handle && dom->ring_ref[console_type] == -1)
+ xengnttab_unmap(xgt_handle, dom->interface[console_type], 1);
else
- munmap(dom->interface, XC_PAGE_SIZE);
- dom->interface = NULL;
- dom->ring_ref = -1;
+ munmap(dom->interface[console_type], XC_PAGE_SIZE);
+ dom->interface[console_type] = NULL;
+ dom->ring_ref[console_type] = -1;
+}
+
+static int bind_event_channel(struct domain *dom, int new_rport, int *lport, int *rport)
+{
+ int err = 0, rc;
+
+ /* Go no further if port has not changed and we are still bound. */
+ if (new_rport == *rport) {
+ xc_evtchn_status_t status = {
+ .dom = DOMID_SELF,
+ .port = *lport };
+ if ((xc_evtchn_status(xc, &status) == 0) &&
+ (status.status == EVTCHNSTAT_interdomain))
+ goto out;
+ }
+
+ *lport = -1;
+ *rport = -1;
+ rc = xenevtchn_bind_interdomain(dom->xce_handle,
+ dom->domid, new_rport);
+
+ if (rc == -1) {
+ err = errno;
+ xenevtchn_close(dom->xce_handle);
+ dom->xce_handle = NULL;
+ goto out;
+ }
+
+ *lport = rc;
+ *rport = new_rport;
+out:
+ return err;
}
static int domain_create_ring(struct domain *dom)
{
- int err, remote_port, ring_ref, rc;
+ int err, remote_port, ring_ref, vcon_remote_port, vcon_ring_ref;
char *type, path[PATH_MAX];
err = xs_gather(xs, dom->conspath,
@@ -547,6 +641,17 @@ static int domain_create_ring(struct domain *dom)
if (err)
goto out;
+ /* vcon console is optional. */
+ err = xs_gather(xs, dom->conspath,
+ "vcon-ring-ref", "%u", &vcon_ring_ref,
+ "vcon-port", "%i", &vcon_remote_port,
+ NULL);
+
+ if (err || vcon_ring_ref == -1)
+ dom->vcon_enabled = false;
+ else
+ dom->vcon_enabled = true;
+
snprintf(path, sizeof(path), "%s/type", dom->conspath);
type = xs_read(xs, XBT_NULL, path, NULL);
if (type && strcmp(type, "xenconsoled") != 0) {
@@ -556,41 +661,51 @@ static int domain_create_ring(struct domain *dom)
free(type);
/* If using ring_ref and it has changed, remap */
- if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
- domain_unmap_interface(dom);
+ if (ring_ref != dom->ring_ref[CONSOLE_TYPE_PV] && dom->ring_ref[CONSOLE_TYPE_PV] != -1)
+ domain_unmap_interface(dom, CONSOLE_TYPE_PV);
+
+ /* If using vcon ring_ref and it has changed, remap */
+ if (dom->vcon_enabled &&
+ vcon_ring_ref != dom->ring_ref[CONSOLE_TYPE_VCON] &&
+ dom->ring_ref[CONSOLE_TYPE_VCON] != -1 )
+ domain_unmap_interface(dom, CONSOLE_TYPE_VCON);
- if (!dom->interface && xgt_handle) {
+ if (!dom->interface[CONSOLE_TYPE_PV] && xgt_handle) {
/* Prefer using grant table */
- dom->interface = xengnttab_map_grant_ref(xgt_handle,
- dom->domid, GNTTAB_RESERVED_CONSOLE,
- PROT_READ|PROT_WRITE);
- dom->ring_ref = -1;
+ dom->interface[CONSOLE_TYPE_PV] = xengnttab_map_grant_ref(xgt_handle,
+ dom->domid, GNTTAB_RESERVED_CONSOLE,
+ PROT_READ|PROT_WRITE);
+ dom->ring_ref[CONSOLE_TYPE_PV] = -1;
}
- if (!dom->interface) {
+
+ if (!dom->interface[CONSOLE_TYPE_PV]) {
/* Fall back to xc_map_foreign_range */
- dom->interface = xc_map_foreign_range(
+ dom->interface[CONSOLE_TYPE_PV] = xc_map_foreign_range(
xc, dom->domid, XC_PAGE_SIZE,
PROT_READ|PROT_WRITE,
(unsigned long)ring_ref);
- if (dom->interface == NULL) {
+ if (dom->interface[CONSOLE_TYPE_PV] == NULL) {
err = EINVAL;
goto out;
}
- dom->ring_ref = ring_ref;
+ dom->ring_ref[CONSOLE_TYPE_PV] = ring_ref;
}
- /* Go no further if port has not changed and we are still bound. */
- if (remote_port == dom->remote_port) {
- xc_evtchn_status_t status = {
- .dom = DOMID_SELF,
- .port = dom->local_port };
- if ((xc_evtchn_status(xc, &status) == 0) &&
- (status.status == EVTCHNSTAT_interdomain))
+ /* Map vcon console ring buffer. */
+ if (dom->vcon_enabled && !dom->interface[CONSOLE_TYPE_VCON]) {
+
+ /* Fall back to xc_map_foreign_range */
+ dom->interface[CONSOLE_TYPE_VCON] = xc_map_foreign_range(
+ xc, dom->domid, XC_PAGE_SIZE,
+ PROT_READ|PROT_WRITE,
+ (unsigned long)vcon_ring_ref);
+ if ( dom->interface[CONSOLE_TYPE_VCON] == NULL ) {
+ err = EINVAL;
goto out;
+ }
+ dom->ring_ref[CONSOLE_TYPE_VCON] = vcon_ring_ref;
}
- dom->local_port = -1;
- dom->remote_port = -1;
if (dom->xce_handle != NULL)
xenevtchn_close(dom->xce_handle);
@@ -602,31 +717,46 @@ static int domain_create_ring(struct domain *dom)
goto out;
}
- rc = xenevtchn_bind_interdomain(dom->xce_handle,
- dom->domid, remote_port);
-
- if (rc == -1) {
- err = errno;
+ err = bind_event_channel(dom, remote_port,
+ &dom->local_port[CONSOLE_TYPE_PV],
+ &dom->remote_port[CONSOLE_TYPE_PV]);
+ if (err)
+ {
xenevtchn_close(dom->xce_handle);
- dom->xce_handle = NULL;
goto out;
}
- dom->local_port = rc;
- dom->remote_port = remote_port;
- if (dom->master_fd == -1) {
+ if (dom->vcon_enabled)
+ {
+ err = bind_event_channel(dom, vcon_remote_port,
+ &dom->local_port[CONSOLE_TYPE_VCON],
+ &dom->remote_port[CONSOLE_TYPE_VCON]);
+ if (err)
+ {
+ xenevtchn_close(dom->xce_handle);
+ goto out;
+ }
+ }
+
+ if (dom->master_fd[CONSOLE_TYPE_PV] == -1) {
if (!domain_create_tty(dom)) {
err = errno;
xenevtchn_close(dom->xce_handle);
dom->xce_handle = NULL;
- dom->local_port = -1;
- dom->remote_port = -1;
+ dom->local_port[CONSOLE_TYPE_PV] = -1;
+ dom->remote_port[CONSOLE_TYPE_PV] = -1;
+ dom->local_port[CONSOLE_TYPE_VCON] = -1;
+ dom->remote_port[CONSOLE_TYPE_VCON] = -1;
+ dom->vcon_enabled = false;
goto out;
}
}
- if (log_guest && (dom->log_fd == -1))
- dom->log_fd = create_domain_log(dom);
+ if (log_guest && (dom->log_fd[CONSOLE_TYPE_PV] == -1))
+ dom->log_fd[CONSOLE_TYPE_PV] = create_domain_log(dom, CONSOLE_TYPE_PV);
+
+ if (log_guest && dom->vcon_enabled && (dom->log_fd[CONSOLE_TYPE_VCON] == -1))
+ dom->log_fd[CONSOLE_TYPE_VCON] = create_domain_log(dom, CONSOLE_TYPE_VCON);
out:
return err;
@@ -681,17 +811,24 @@ static struct domain *create_domain(int domid)
dom->conspath = s;
strcat(dom->conspath, "/console");
- dom->master_fd = -1;
- dom->master_pollfd_idx = -1;
- dom->slave_fd = -1;
- dom->log_fd = -1;
+ dom->master_fd[CONSOLE_TYPE_PV] = -1;
+ dom->master_pollfd_idx[CONSOLE_TYPE_PV] = -1;
+ dom->slave_fd[CONSOLE_TYPE_PV] = -1;
+ dom->master_fd[CONSOLE_TYPE_VCON] = -1;
+ dom->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
+ dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
+ dom->log_fd[CONSOLE_TYPE_PV] = -1;
+ dom->log_fd[CONSOLE_TYPE_VCON] = -1;
dom->xce_pollfd_idx = -1;
dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
- dom->ring_ref = -1;
- dom->local_port = -1;
- dom->remote_port = -1;
+ dom->ring_ref[CONSOLE_TYPE_PV] = -1;
+ dom->ring_ref[CONSOLE_TYPE_VCON] = -1;
+ dom->local_port[CONSOLE_TYPE_PV] = -1;
+ dom->remote_port[CONSOLE_TYPE_PV] = -1;
+ dom->local_port[CONSOLE_TYPE_VCON] = -1;
+ dom->remote_port[CONSOLE_TYPE_VCON] = -1;
if (!watch_domain(dom, true))
goto out;
@@ -737,13 +874,21 @@ static void cleanup_domain(struct domain *d)
{
domain_close_tty(d);
- if (d->log_fd != -1) {
- close(d->log_fd);
- d->log_fd = -1;
+ if (d->log_fd[CONSOLE_TYPE_PV] != -1) {
+ close(d->log_fd[CONSOLE_TYPE_PV]);
+ d->log_fd[CONSOLE_TYPE_PV] = -1;
+ }
+
+ if (d->log_fd[CONSOLE_TYPE_VCON] != -1) {
+ close(d->log_fd[CONSOLE_TYPE_VCON]);
+ d->log_fd[CONSOLE_TYPE_VCON] = -1;
}
- free(d->buffer.data);
- d->buffer.data = NULL;
+ free(d->buffer[CONSOLE_TYPE_PV].data);
+ free(d->buffer[CONSOLE_TYPE_VCON].data);
+
+ d->buffer[CONSOLE_TYPE_PV].data = NULL;
+ d->buffer[CONSOLE_TYPE_VCON].data = NULL;
free(d->conspath);
d->conspath = NULL;
@@ -755,7 +900,8 @@ static void shutdown_domain(struct domain *d)
{
d->is_dead = true;
watch_domain(d, false);
- domain_unmap_interface(d);
+ domain_unmap_interface(d, CONSOLE_TYPE_PV);
+ domain_unmap_interface(d, CONSOLE_TYPE_VCON);
if (d->xce_handle != NULL)
xenevtchn_close(d->xce_handle);
d->xce_handle = NULL;
@@ -786,9 +932,9 @@ static void enum_domains(void)
}
}
-static int ring_free_bytes(struct domain *dom)
+static int ring_free_bytes(struct domain *dom, int console_type)
{
- struct xencons_interface *intf = dom->interface;
+ struct xencons_interface *intf = dom->interface[console_type];
XENCONS_RING_IDX cons, prod, space;
cons = intf->in_cons;
@@ -813,25 +959,26 @@ static void domain_handle_broken_tty(struct domain *dom, int recreate)
}
}
-static void handle_tty_read(struct domain *dom)
+static void handle_tty_read(struct domain *dom, int console_type)
{
ssize_t len = 0;
char msg[80];
int i;
- struct xencons_interface *intf = dom->interface;
XENCONS_RING_IDX prod;
+ struct xencons_interface *intf=dom->interface[console_type];
+ xenevtchn_port_or_error_t port=dom->local_port[console_type];
if (dom->is_dead)
return;
- len = ring_free_bytes(dom);
+ len = ring_free_bytes(dom, console_type);
if (len == 0)
return;
if (len > sizeof(msg))
len = sizeof(msg);
- len = read(dom->master_fd, msg, len);
+ len = read(dom->master_fd[console_type], msg, len);
/*
* Note: on Solaris, len == 0 means the slave closed, and this
* is no problem, but Linux can't handle this usefully, so we
@@ -847,28 +994,29 @@ static void handle_tty_read(struct domain *dom)
}
xen_wmb();
intf->in_prod = prod;
- xenevtchn_notify(dom->xce_handle, dom->local_port);
+ xenevtchn_notify(dom->xce_handle, port);
} else {
domain_close_tty(dom);
shutdown_domain(dom);
}
}
-static void handle_tty_write(struct domain *dom)
+static void handle_tty_write(struct domain *dom, int console_type)
{
ssize_t len;
if (dom->is_dead)
return;
- len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
- dom->buffer.size - dom->buffer.consumed);
+ len = write(dom->master_fd[console_type],
+ dom->buffer[console_type].data + dom->buffer[console_type].consumed,
+ dom->buffer[console_type].size - dom->buffer[console_type].consumed);
if (len < 1) {
dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
dom->domid, len, errno);
domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
} else {
- buffer_advance(&dom->buffer, len);
+ buffer_advance(&dom->buffer[console_type], len);
}
}
@@ -884,7 +1032,10 @@ static void handle_ring_read(struct domain *dom)
dom->event_count++;
- buffer_append(dom);
+ if (port == dom->local_port[CONSOLE_TYPE_VCON])
+ buffer_append(dom, CONSOLE_TYPE_VCON);
+ else
+ buffer_append(dom, CONSOLE_TYPE_PV);
if (dom->event_count < RATE_LIMIT_ALLOWANCE)
(void)xenevtchn_unmask(dom->xce_handle, port);
@@ -954,9 +1105,16 @@ static void handle_log_reload(void)
if (log_guest) {
struct domain *d;
for (d = dom_head; d; d = d->next) {
- if (d->log_fd != -1)
- close(d->log_fd);
- d->log_fd = create_domain_log(d);
+ if (d->log_fd[CONSOLE_TYPE_PV] != -1)
+ close(d->log_fd[CONSOLE_TYPE_PV]);
+ d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_PV);
+
+ if (d->vcon_enabled)
+ {
+ if (d->log_fd[CONSOLE_TYPE_VCON] != -1)
+ close(d->log_fd[CONSOLE_TYPE_VCON]);
+ d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_VCON);
+ }
}
}
@@ -1074,7 +1232,9 @@ void handle_io(void)
if ((now+5) > d->next_period) {
d->next_period = now + RATE_LIMIT_PERIOD;
if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
- (void)xenevtchn_unmask(d->xce_handle, d->local_port);
+ (void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_PV]);
+ if (d->vcon_enabled)
+ (void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_VCON]);
}
d->event_count = 0;
}
@@ -1087,27 +1247,47 @@ void handle_io(void)
d->next_period < next_timeout)
next_timeout = d->next_period;
} else if (d->xce_handle != NULL) {
- if (discard_overflowed_data ||
- !d->buffer.max_capacity ||
- d->buffer.size < d->buffer.max_capacity) {
+
+ if (buffer_available(d, CONSOLE_TYPE_PV)) {
+ int evtchn_fd = xenevtchn_fd(d->xce_handle);
+ d->xce_pollfd_idx = set_fds(evtchn_fd,
+ POLLIN|POLLPRI);
+ }
+
+ if (buffer_available(d, CONSOLE_TYPE_VCON ) )
+ {
int evtchn_fd = xenevtchn_fd(d->xce_handle);
d->xce_pollfd_idx = set_fds(evtchn_fd,
- POLLIN|POLLPRI);
+ POLLIN|POLLPRI);
}
}
- if (d->master_fd != -1) {
+ if (d->master_fd[CONSOLE_TYPE_PV] != -1) {
short events = 0;
- if (!d->is_dead && ring_free_bytes(d))
+ if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_PV))
events |= POLLIN;
- if (!buffer_empty(&d->buffer))
+ if (!buffer_empty(&d->buffer[CONSOLE_TYPE_PV]))
events |= POLLOUT;
if (events)
- d->master_pollfd_idx =
- set_fds(d->master_fd,
- events|POLLPRI);
+ d->master_pollfd_idx[CONSOLE_TYPE_PV] =
+ set_fds(d->master_fd[CONSOLE_TYPE_PV],
+ events|POLLPRI);
+ }
+
+ if (d->vcon_enabled && d->master_fd[CONSOLE_TYPE_VCON] != -1) {
+ short events = 0;
+ if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_VCON))
+ events |= POLLIN;
+
+ if (!buffer_empty(&d->buffer[CONSOLE_TYPE_VCON]))
+ events |= POLLOUT;
+
+ if (events)
+ d->master_pollfd_idx[CONSOLE_TYPE_VCON] =
+ set_fds(d->master_fd[CONSOLE_TYPE_VCON],
+ events|POLLPRI);
}
}
@@ -1166,6 +1346,16 @@ void handle_io(void)
for (d = dom_head; d; d = n) {
n = d->next;
+
+ /*
+ * Check if the data pending flag is set for any of the consoles.
+ * If yes then service those first.
+ */
+ if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
+ buffer_append(d, CONSOLE_TYPE_PV);
+ else if ( d->console_data_pending & (1<<CONSOLE_TYPE_VCON) )
+ buffer_append(d, CONSOLE_TYPE_VCON);
+
if (d->event_count < RATE_LIMIT_ALLOWANCE) {
if (d->xce_handle != NULL &&
d->xce_pollfd_idx != -1 &&
@@ -1176,22 +1366,36 @@ void handle_io(void)
handle_ring_read(d);
}
- if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
- if (fds[d->master_pollfd_idx].revents &
+ if (d->master_fd[CONSOLE_TYPE_PV] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_PV] != -1) {
+ if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
+ ~(POLLIN|POLLOUT|POLLPRI))
+ domain_handle_broken_tty(d, domain_is_valid(d->domid));
+ else {
+ if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
+ POLLIN)
+ handle_tty_read(d, CONSOLE_TYPE_PV);
+ if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
+ POLLOUT)
+ handle_tty_write(d, CONSOLE_TYPE_PV);
+ }
+ }
+
+ if (d->master_fd[CONSOLE_TYPE_VCON] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_VCON] != -1) {
+ if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
~(POLLIN|POLLOUT|POLLPRI))
- domain_handle_broken_tty(d,
- domain_is_valid(d->domid));
+ domain_handle_broken_tty(d, domain_is_valid(d->domid));
else {
- if (fds[d->master_pollfd_idx].revents &
+ if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
POLLIN)
- handle_tty_read(d);
- if (fds[d->master_pollfd_idx].revents &
+ handle_tty_read(d, CONSOLE_TYPE_VCON);
+ if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
POLLOUT)
- handle_tty_write(d);
+ handle_tty_write(d, CONSOLE_TYPE_VCON);
}
}
- d->xce_pollfd_idx = d->master_pollfd_idx = -1;
+ d->xce_pollfd_idx = d->master_pollfd_idx[CONSOLE_TYPE_PV] =
+ d->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
if (d->last_seen != enum_pass)
shutdown_domain(d);
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles
2017-04-03 9:44 ` [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles Bhupinder Thakur
@ 2017-04-12 16:33 ` Wei Liu
2017-04-24 14:52 ` Bhupinder Thakur
0 siblings, 1 reply; 53+ messages in thread
From: Wei Liu @ 2017-04-12 16:33 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, Apr 03, 2017 at 03:14:31PM +0530, Bhupinder Thakur wrote:
> Xenconsole supports only PV console currently. To get access to emulated pl011
> uart another backend console is required.
>
> This patch modifies different data structures and APIs used
> in xenconsole to support two console types: PV and VCON (virtual console).
>
> Change summary:
>
> 1. Modify the domain structure to support two console types: PV and a
> virtual console (VCON).
>
> 2. Modify different APIs such as buffer_append() to take a new parameter
> console_type as input and operate on the data structures indexed by
> the console_type.
>
> 3. Modfications in domain_create_ring():
> - Bind to the vpl011 event channel obtained from the xen store as a
> new
> parameter
> - Map the PFN to its address space to be used as IN/OUT ring
> buffers.
> It obtains the PFN from the xen store as a new parameter
>
> 4. Modifications in handle_ring_read() to handle both PV and VCON
> events.
>
> 5. Add a new log_file for VCON console logs.
>
It seems that this patch and previous one should be merged into one.
There are a lot of coding style issues, please fix all of them.
The code looks rather repetitive unfortunately. I believe a lot of the
repetitiveness can be avoided by using loops and pointers.
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> tools/console/daemon/io.c | 508 ++++++++++++++++++++++++++++++++--------------
> 1 file changed, 356 insertions(+), 152 deletions(-)
>
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 0cd1fee..b9be5a5 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -164,11 +164,39 @@ static int write_with_timestamp(int fd, const char *data, size_t sz,
> return 0;
> }
>
> -static void buffer_append(struct domain *dom)
> +/*
> + * This function checks if the data is allowed to be buffered for that console.
> + * If not then it marks it pending for later receiving.
> + */
> +static bool buffer_available(struct domain *dom, int console_type)
> +{
> + if (discard_overflowed_data ||
> + !dom->buffer[console_type].max_capacity ||
> + dom->buffer[console_type].size < dom->buffer[console_type].max_capacity)
Line too long.
> + {
> + dom->console_data_pending &= ~(1<<console_type);
> + return true;
> + }
> + else
> + {
> + dom->console_data_pending |= (1<<console_type);
> + return false;
> + }
> +}
> +
> +static void buffer_append(struct domain *dom, int console_type)
> {
> - struct buffer *buffer = &dom->buffer;
> + struct buffer *buffer = &dom->buffer[console_type];
> + struct xencons_interface *intf=dom->interface[console_type];
> + xenevtchn_port_or_error_t port=dom->local_port[console_type];
Spaces around "=".
> +
> XENCONS_RING_IDX cons, prod, size;
> - struct xencons_interface *intf = dom->interface;
> +
> + /*
> + * check if more data is allowed to be buffered
> + */
> + if (!buffer_available(dom, console_type))
> + return;
>
> cons = intf->out_cons;
> prod = intf->out_prod;
> @@ -193,22 +221,22 @@ static void buffer_append(struct domain *dom)
>
> xen_mb();
> intf->out_cons = cons;
> - xenevtchn_notify(dom->xce_handle, dom->local_port);
> + xenevtchn_notify(dom->xce_handle, port);
>
> /* Get the data to the logfile as early as possible because if
> * no one is listening on the console pty then it will fill up
> * and handle_tty_write will stop being called.
> */
> - if (dom->log_fd != -1) {
> + if (dom->log_fd[console_type] != -1) {
> int logret;
> if (log_time_guest) {
> logret = write_with_timestamp(
> - dom->log_fd,
> + dom->log_fd[console_type],
> buffer->data + buffer->size - size,
> size, &log_time_guest_needts);
> } else {
> logret = write_all(
> - dom->log_fd,
> + dom->log_fd[console_type],
> buffer->data + buffer->size - size,
> size);
> }
> @@ -296,12 +324,13 @@ static int create_hv_log(void)
> return fd;
> }
>
> -static int create_domain_log(struct domain *dom)
> +static int create_domain_log(struct domain *dom, int console_type)
> {
> char logfile[PATH_MAX];
> char *namepath, *data, *s;
> int fd;
> unsigned int len;
> + char *console_name[]={"pv", "vcon"};
>
> namepath = xs_get_domain_path(xs, dom->domid);
> s = realloc(namepath, strlen(namepath) + 6);
> @@ -320,7 +349,7 @@ static int create_domain_log(struct domain *dom)
> return -1;
> }
>
> - snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
> + snprintf(logfile, PATH_MAX-1, "%s/guest-%s-%s.log", log_dir, console_name[console_type], data);
Line too long.
> free(data);
> logfile[PATH_MAX-1] = '\0';
>
> @@ -344,14 +373,24 @@ static int create_domain_log(struct domain *dom)
>
> static void domain_close_tty(struct domain *dom)
> {
> - if (dom->master_fd != -1) {
> - close(dom->master_fd);
> - dom->master_fd = -1;
> + if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
> + close(dom->master_fd[CONSOLE_TYPE_PV]);
> + dom->master_fd[CONSOLE_TYPE_PV] = -1;
> + }
> +
> + if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
> + close(dom->slave_fd[CONSOLE_TYPE_PV]);
> + dom->slave_fd[CONSOLE_TYPE_PV] = -1;
> + }
> +
> + if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
> + close(dom->master_fd[CONSOLE_TYPE_VCON]);
> + dom->master_fd[CONSOLE_TYPE_VCON] = -1;
> }
>
> - if (dom->slave_fd != -1) {
> - close(dom->slave_fd);
> - dom->slave_fd = -1;
> + if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
> + close(dom->slave_fd[CONSOLE_TYPE_VCON]);
> + dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
> }
You can use two loops to avoid repetitive snippets. But I'm fine with
the code as-is, too.
> }
>
> @@ -424,63 +463,86 @@ static int domain_create_tty(struct domain *dom)
> char *data;
> unsigned int len;
> struct termios term;
> + int console_type=0;
>
> - assert(dom->slave_fd == -1);
> - assert(dom->master_fd == -1);
> + assert(dom->master_fd[CONSOLE_TYPE_PV] == -1);
> + assert(dom->slave_fd[CONSOLE_TYPE_PV] == -1);
> + assert(dom->master_fd[CONSOLE_TYPE_VCON] == -1);
> + assert(dom->slave_fd[CONSOLE_TYPE_VCON] == -1);
>
> - if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
> - err = errno;
> - dolog(LOG_ERR, "Failed to create tty for domain-%d "
> - "(errno = %i, %s)",
> - dom->domid, err, strerror(err));
> - return 0;
> - }
> + /*
> + * Open pty for both PV and vcon consoles.
> + */
> + for (console_type=0; console_type<MAX_CONSOLE; console_type++)
Spaces around "=" and "<".
> + {
> + if (openpty(&dom->master_fd[console_type], &dom->slave_fd[console_type], NULL, NULL, NULL) < 0) {
Line too long
> + err = errno;
> + dolog(LOG_ERR, "Failed to create tty for domain-%d "
> + "(errno = %i, %s)",
> + dom->domid, err, strerror(err));
> + return 0;
> + }
>
> - if (tcgetattr(dom->slave_fd, &term) < 0) {
> - err = errno;
> - dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
> - "(errno = %i, %s)",
> - dom->domid, err, strerror(err));
> - goto out;
> - }
> - cfmakeraw(&term);
> - if (tcsetattr(dom->slave_fd, TCSANOW, &term) < 0) {
> - err = errno;
> - dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
> - "(errno = %i, %s)",
> - dom->domid, err, strerror(err));
> - goto out;
> - }
> + if (tcgetattr(dom->slave_fd[console_type], &term) < 0) {
> + err = errno;
> + dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
> + "(errno = %i, %s)",
> + dom->domid, err, strerror(err));
> + goto out;
> + }
> + cfmakeraw(&term);
> + if (tcsetattr(dom->slave_fd[console_type], TCSANOW, &term) < 0) {
> + err = errno;
> + dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
> + "(errno = %i, %s)",
> + dom->domid, err, strerror(err));
> + goto out;
> + }
>
> - if ((slave = ptsname(dom->master_fd)) == NULL) {
> - err = errno;
> - dolog(LOG_ERR, "Failed to get slave name for domain-%d "
> - "(errno = %i, %s)",
> - dom->domid, err, strerror(err));
> - goto out;
> - }
> + if ((slave = ptsname(dom->master_fd[console_type])) == NULL) {
> + err = errno;
> + dolog(LOG_ERR, "Failed to get slave name for domain-%d "
> + "(errno = %i, %s)",
> + dom->domid, err, strerror(err));
> + goto out;
> + }
>
> - success = asprintf(&path, "%s/limit", dom->conspath) !=
> - -1;
> - if (!success)
> - goto out;
> - data = xs_read(xs, XBT_NULL, path, &len);
> - if (data) {
> - dom->buffer.max_capacity = strtoul(data, 0, 0);
> - free(data);
> + /*
> + * Initialize the console buffer capacity.
> + */
> + success = asprintf(&path, "%s/limit", dom->conspath) !=
> + -1;
> + if (!success)
> + goto out;
> + data = xs_read(xs, XBT_NULL, path, &len);
> + if (data) {
> + dom->buffer[console_type].max_capacity = strtoul(data, 0, 0);
> + free(data);
> + }
> + free(path);
> +
> + /*
> + * Write console slave name to xen store.
> + */
> + if (console_type == CONSOLE_TYPE_VCON)
> + success = (asprintf(&path, "%s/vtty", dom->conspath) != -1);
> + else
> + success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
> +
> + if (!success)
> + goto out;
> + success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
> + free(path);
> +
> + if (fcntl(dom->master_fd[console_type], F_SETFL, O_NONBLOCK) == -1)
> + goto out;
> +
> + if (!dom->vcon_enabled)
> + break;
> }
> - free(path);
>
> - success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
> if (!success)
> goto out;
> - success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
> - free(path);
> - if (!success)
> - goto out;
> -
> - if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
> - goto out;
>
> return 1;
> out:
> @@ -523,21 +585,53 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
> return ret;
> }
>
> -static void domain_unmap_interface(struct domain *dom)
> +static void domain_unmap_interface(struct domain *dom, int console_type)
> {
> - if (dom->interface == NULL)
> + if (dom->interface[console_type] == NULL)
> return;
> - if (xgt_handle && dom->ring_ref == -1)
> - xengnttab_unmap(xgt_handle, dom->interface, 1);
> + if (xgt_handle && dom->ring_ref[console_type] == -1)
> + xengnttab_unmap(xgt_handle, dom->interface[console_type], 1);
> else
> - munmap(dom->interface, XC_PAGE_SIZE);
> - dom->interface = NULL;
> - dom->ring_ref = -1;
> + munmap(dom->interface[console_type], XC_PAGE_SIZE);
> + dom->interface[console_type] = NULL;
> + dom->ring_ref[console_type] = -1;
> +}
> +
> +static int bind_event_channel(struct domain *dom, int new_rport, int *lport, int *rport)
Line too long.
> +{
> + int err = 0, rc;
> +
> + /* Go no further if port has not changed and we are still bound. */
> + if (new_rport == *rport) {
> + xc_evtchn_status_t status = {
> + .dom = DOMID_SELF,
> + .port = *lport };
> + if ((xc_evtchn_status(xc, &status) == 0) &&
> + (status.status == EVTCHNSTAT_interdomain))
> + goto out;
> + }
> +
> + *lport = -1;
> + *rport = -1;
> + rc = xenevtchn_bind_interdomain(dom->xce_handle,
> + dom->domid, new_rport);
Indentation.
> +
> + if (rc == -1) {
> + err = errno;
> + xenevtchn_close(dom->xce_handle);
> + dom->xce_handle = NULL;
> + goto out;
> + }
> +
> + *lport = rc;
> + *rport = new_rport;
> +out:
> + return err;
> }
>
> static int domain_create_ring(struct domain *dom)
> {
> - int err, remote_port, ring_ref, rc;
> + int err, remote_port, ring_ref, vcon_remote_port, vcon_ring_ref;
> char *type, path[PATH_MAX];
>
> err = xs_gather(xs, dom->conspath,
> @@ -547,6 +641,17 @@ static int domain_create_ring(struct domain *dom)
> if (err)
> goto out;
>
> + /* vcon console is optional. */
> + err = xs_gather(xs, dom->conspath,
> + "vcon-ring-ref", "%u", &vcon_ring_ref,
> + "vcon-port", "%i", &vcon_remote_port,
> + NULL);
> +
Indentation.
> + if (err || vcon_ring_ref == -1)
> + dom->vcon_enabled = false;
> + else
> + dom->vcon_enabled = true;
> +
> snprintf(path, sizeof(path), "%s/type", dom->conspath);
> type = xs_read(xs, XBT_NULL, path, NULL);
> if (type && strcmp(type, "xenconsoled") != 0) {
> @@ -556,41 +661,51 @@ static int domain_create_ring(struct domain *dom)
> free(type);
>
> /* If using ring_ref and it has changed, remap */
> - if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
> - domain_unmap_interface(dom);
> + if (ring_ref != dom->ring_ref[CONSOLE_TYPE_PV] && dom->ring_ref[CONSOLE_TYPE_PV] != -1)
Line too long.
> + domain_unmap_interface(dom, CONSOLE_TYPE_PV);
> +
> + /* If using vcon ring_ref and it has changed, remap */
> + if (dom->vcon_enabled &&
> + vcon_ring_ref != dom->ring_ref[CONSOLE_TYPE_VCON] &&
> + dom->ring_ref[CONSOLE_TYPE_VCON] != -1 )
> + domain_unmap_interface(dom, CONSOLE_TYPE_VCON);
Indentation.
>
> - if (!dom->interface && xgt_handle) {
> + if (!dom->interface[CONSOLE_TYPE_PV] && xgt_handle) {
> /* Prefer using grant table */
> - dom->interface = xengnttab_map_grant_ref(xgt_handle,
> - dom->domid, GNTTAB_RESERVED_CONSOLE,
> - PROT_READ|PROT_WRITE);
> - dom->ring_ref = -1;
> + dom->interface[CONSOLE_TYPE_PV] = xengnttab_map_grant_ref(xgt_handle,
> + dom->domid, GNTTAB_RESERVED_CONSOLE,
> + PROT_READ|PROT_WRITE);
> + dom->ring_ref[CONSOLE_TYPE_PV] = -1;
Indentation.
> }
> - if (!dom->interface) {
> +
> + if (!dom->interface[CONSOLE_TYPE_PV]) {
> /* Fall back to xc_map_foreign_range */
> - dom->interface = xc_map_foreign_range(
> + dom->interface[CONSOLE_TYPE_PV] = xc_map_foreign_range(
> xc, dom->domid, XC_PAGE_SIZE,
> PROT_READ|PROT_WRITE,
> (unsigned long)ring_ref);
> - if (dom->interface == NULL) {
> + if (dom->interface[CONSOLE_TYPE_PV] == NULL) {
> err = EINVAL;
> goto out;
> }
> - dom->ring_ref = ring_ref;
> + dom->ring_ref[CONSOLE_TYPE_PV] = ring_ref;
> }
>
> - /* Go no further if port has not changed and we are still bound. */
> - if (remote_port == dom->remote_port) {
> - xc_evtchn_status_t status = {
> - .dom = DOMID_SELF,
> - .port = dom->local_port };
> - if ((xc_evtchn_status(xc, &status) == 0) &&
> - (status.status == EVTCHNSTAT_interdomain))
> + /* Map vcon console ring buffer. */
> + if (dom->vcon_enabled && !dom->interface[CONSOLE_TYPE_VCON]) {
> +
> + /* Fall back to xc_map_foreign_range */
> + dom->interface[CONSOLE_TYPE_VCON] = xc_map_foreign_range(
> + xc, dom->domid, XC_PAGE_SIZE,
> + PROT_READ|PROT_WRITE,
> + (unsigned long)vcon_ring_ref);
> + if ( dom->interface[CONSOLE_TYPE_VCON] == NULL ) {
> + err = EINVAL;
> goto out;
> + }
> + dom->ring_ref[CONSOLE_TYPE_VCON] = vcon_ring_ref;
> }
>
> - dom->local_port = -1;
> - dom->remote_port = -1;
> if (dom->xce_handle != NULL)
> xenevtchn_close(dom->xce_handle);
>
> @@ -602,31 +717,46 @@ static int domain_create_ring(struct domain *dom)
> goto out;
> }
>
> - rc = xenevtchn_bind_interdomain(dom->xce_handle,
> - dom->domid, remote_port);
> -
> - if (rc == -1) {
> - err = errno;
> + err = bind_event_channel(dom, remote_port,
> + &dom->local_port[CONSOLE_TYPE_PV],
> + &dom->remote_port[CONSOLE_TYPE_PV]);
Indentation.
> + if (err)
> + {
> xenevtchn_close(dom->xce_handle);
> - dom->xce_handle = NULL;
> goto out;
> }
> - dom->local_port = rc;
> - dom->remote_port = remote_port;
>
> - if (dom->master_fd == -1) {
> + if (dom->vcon_enabled)
> + {
> + err = bind_event_channel(dom, vcon_remote_port,
> + &dom->local_port[CONSOLE_TYPE_VCON],
> + &dom->remote_port[CONSOLE_TYPE_VCON]);
Indentation.
> + if (err)
> + {
> + xenevtchn_close(dom->xce_handle);
> + goto out;
> + }
> + }
> +
> + if (dom->master_fd[CONSOLE_TYPE_PV] == -1) {
> if (!domain_create_tty(dom)) {
> err = errno;
> xenevtchn_close(dom->xce_handle);
> dom->xce_handle = NULL;
> - dom->local_port = -1;
> - dom->remote_port = -1;
> + dom->local_port[CONSOLE_TYPE_PV] = -1;
> + dom->remote_port[CONSOLE_TYPE_PV] = -1;
> + dom->local_port[CONSOLE_TYPE_VCON] = -1;
> + dom->remote_port[CONSOLE_TYPE_VCON] = -1;
> + dom->vcon_enabled = false;
> goto out;
> }
> }
>
> - if (log_guest && (dom->log_fd == -1))
> - dom->log_fd = create_domain_log(dom);
> + if (log_guest && (dom->log_fd[CONSOLE_TYPE_PV] == -1))
> + dom->log_fd[CONSOLE_TYPE_PV] = create_domain_log(dom, CONSOLE_TYPE_PV);
> +
> + if (log_guest && dom->vcon_enabled && (dom->log_fd[CONSOLE_TYPE_VCON] == -1))
> + dom->log_fd[CONSOLE_TYPE_VCON] = create_domain_log(dom, CONSOLE_TYPE_VCON);
>
> out:
> return err;
> @@ -681,17 +811,24 @@ static struct domain *create_domain(int domid)
> dom->conspath = s;
> strcat(dom->conspath, "/console");
>
> - dom->master_fd = -1;
> - dom->master_pollfd_idx = -1;
> - dom->slave_fd = -1;
> - dom->log_fd = -1;
> + dom->master_fd[CONSOLE_TYPE_PV] = -1;
> + dom->master_pollfd_idx[CONSOLE_TYPE_PV] = -1;
> + dom->slave_fd[CONSOLE_TYPE_PV] = -1;
> + dom->master_fd[CONSOLE_TYPE_VCON] = -1;
> + dom->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
> + dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
> + dom->log_fd[CONSOLE_TYPE_PV] = -1;
> + dom->log_fd[CONSOLE_TYPE_VCON] = -1;
> dom->xce_pollfd_idx = -1;
>
> dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
>
> - dom->ring_ref = -1;
> - dom->local_port = -1;
> - dom->remote_port = -1;
> + dom->ring_ref[CONSOLE_TYPE_PV] = -1;
> + dom->ring_ref[CONSOLE_TYPE_VCON] = -1;
> + dom->local_port[CONSOLE_TYPE_PV] = -1;
> + dom->remote_port[CONSOLE_TYPE_PV] = -1;
> + dom->local_port[CONSOLE_TYPE_VCON] = -1;
> + dom->remote_port[CONSOLE_TYPE_VCON] = -1;
>
> if (!watch_domain(dom, true))
> goto out;
> @@ -737,13 +874,21 @@ static void cleanup_domain(struct domain *d)
> {
> domain_close_tty(d);
>
> - if (d->log_fd != -1) {
> - close(d->log_fd);
> - d->log_fd = -1;
> + if (d->log_fd[CONSOLE_TYPE_PV] != -1) {
> + close(d->log_fd[CONSOLE_TYPE_PV]);
> + d->log_fd[CONSOLE_TYPE_PV] = -1;
> + }
> +
> + if (d->log_fd[CONSOLE_TYPE_VCON] != -1) {
> + close(d->log_fd[CONSOLE_TYPE_VCON]);
> + d->log_fd[CONSOLE_TYPE_VCON] = -1;
> }
>
> - free(d->buffer.data);
> - d->buffer.data = NULL;
> + free(d->buffer[CONSOLE_TYPE_PV].data);
> + free(d->buffer[CONSOLE_TYPE_VCON].data);
> +
> + d->buffer[CONSOLE_TYPE_PV].data = NULL;
> + d->buffer[CONSOLE_TYPE_VCON].data = NULL;
>
> free(d->conspath);
> d->conspath = NULL;
> @@ -755,7 +900,8 @@ static void shutdown_domain(struct domain *d)
> {
> d->is_dead = true;
> watch_domain(d, false);
> - domain_unmap_interface(d);
> + domain_unmap_interface(d, CONSOLE_TYPE_PV);
> + domain_unmap_interface(d, CONSOLE_TYPE_VCON);
> if (d->xce_handle != NULL)
> xenevtchn_close(d->xce_handle);
> d->xce_handle = NULL;
> @@ -786,9 +932,9 @@ static void enum_domains(void)
> }
> }
>
> -static int ring_free_bytes(struct domain *dom)
> +static int ring_free_bytes(struct domain *dom, int console_type)
> {
> - struct xencons_interface *intf = dom->interface;
> + struct xencons_interface *intf = dom->interface[console_type];
> XENCONS_RING_IDX cons, prod, space;
>
> cons = intf->in_cons;
> @@ -813,25 +959,26 @@ static void domain_handle_broken_tty(struct domain *dom, int recreate)
> }
> }
>
> -static void handle_tty_read(struct domain *dom)
> +static void handle_tty_read(struct domain *dom, int console_type)
> {
> ssize_t len = 0;
> char msg[80];
> int i;
> - struct xencons_interface *intf = dom->interface;
> XENCONS_RING_IDX prod;
> + struct xencons_interface *intf=dom->interface[console_type];
> + xenevtchn_port_or_error_t port=dom->local_port[console_type];
>
> if (dom->is_dead)
> return;
>
> - len = ring_free_bytes(dom);
> + len = ring_free_bytes(dom, console_type);
> if (len == 0)
> return;
>
> if (len > sizeof(msg))
> len = sizeof(msg);
>
> - len = read(dom->master_fd, msg, len);
> + len = read(dom->master_fd[console_type], msg, len);
> /*
> * Note: on Solaris, len == 0 means the slave closed, and this
> * is no problem, but Linux can't handle this usefully, so we
> @@ -847,28 +994,29 @@ static void handle_tty_read(struct domain *dom)
> }
> xen_wmb();
> intf->in_prod = prod;
> - xenevtchn_notify(dom->xce_handle, dom->local_port);
> + xenevtchn_notify(dom->xce_handle, port);
> } else {
> domain_close_tty(dom);
> shutdown_domain(dom);
> }
> }
>
> -static void handle_tty_write(struct domain *dom)
> +static void handle_tty_write(struct domain *dom, int console_type)
> {
> ssize_t len;
>
> if (dom->is_dead)
> return;
>
> - len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
> - dom->buffer.size - dom->buffer.consumed);
> + len = write(dom->master_fd[console_type],
> + dom->buffer[console_type].data + dom->buffer[console_type].consumed,
> + dom->buffer[console_type].size - dom->buffer[console_type].consumed);
> if (len < 1) {
> dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
> dom->domid, len, errno);
> domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
> } else {
> - buffer_advance(&dom->buffer, len);
> + buffer_advance(&dom->buffer[console_type], len);
> }
> }
>
> @@ -884,7 +1032,10 @@ static void handle_ring_read(struct domain *dom)
>
> dom->event_count++;
>
> - buffer_append(dom);
> + if (port == dom->local_port[CONSOLE_TYPE_VCON])
> + buffer_append(dom, CONSOLE_TYPE_VCON);
> + else
> + buffer_append(dom, CONSOLE_TYPE_PV);
>
> if (dom->event_count < RATE_LIMIT_ALLOWANCE)
> (void)xenevtchn_unmask(dom->xce_handle, port);
> @@ -954,9 +1105,16 @@ static void handle_log_reload(void)
> if (log_guest) {
> struct domain *d;
> for (d = dom_head; d; d = d->next) {
> - if (d->log_fd != -1)
> - close(d->log_fd);
> - d->log_fd = create_domain_log(d);
> + if (d->log_fd[CONSOLE_TYPE_PV] != -1)
> + close(d->log_fd[CONSOLE_TYPE_PV]);
> + d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_PV);
> +
> + if (d->vcon_enabled)
> + {
> + if (d->log_fd[CONSOLE_TYPE_VCON] != -1)
> + close(d->log_fd[CONSOLE_TYPE_VCON]);
> + d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_VCON);
Line too long.
> + }
> }
> }
>
> @@ -1074,7 +1232,9 @@ void handle_io(void)
> if ((now+5) > d->next_period) {
> d->next_period = now + RATE_LIMIT_PERIOD;
> if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
> - (void)xenevtchn_unmask(d->xce_handle, d->local_port);
> + (void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_PV]);
> + if (d->vcon_enabled)
> + (void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_VCON]);
Line too long.
> }
> d->event_count = 0;
> }
> @@ -1087,27 +1247,47 @@ void handle_io(void)
> d->next_period < next_timeout)
> next_timeout = d->next_period;
> } else if (d->xce_handle != NULL) {
> - if (discard_overflowed_data ||
> - !d->buffer.max_capacity ||
> - d->buffer.size < d->buffer.max_capacity) {
> +
> + if (buffer_available(d, CONSOLE_TYPE_PV)) {
> + int evtchn_fd = xenevtchn_fd(d->xce_handle);
> + d->xce_pollfd_idx = set_fds(evtchn_fd,
> + POLLIN|POLLPRI);
Indentation.
> + }
> +
> + if (buffer_available(d, CONSOLE_TYPE_VCON ) )
Extraneous space after CONSOLE_TYPE_VCON.
> + {
> int evtchn_fd = xenevtchn_fd(d->xce_handle);
> d->xce_pollfd_idx = set_fds(evtchn_fd,
> - POLLIN|POLLPRI);
> + POLLIN|POLLPRI);
Indentation.
> }
> }
>
> - if (d->master_fd != -1) {
> + if (d->master_fd[CONSOLE_TYPE_PV] != -1) {
> short events = 0;
> - if (!d->is_dead && ring_free_bytes(d))
> + if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_PV))
> events |= POLLIN;
>
> - if (!buffer_empty(&d->buffer))
> + if (!buffer_empty(&d->buffer[CONSOLE_TYPE_PV]))
> events |= POLLOUT;
>
> if (events)
> - d->master_pollfd_idx =
> - set_fds(d->master_fd,
> - events|POLLPRI);
> + d->master_pollfd_idx[CONSOLE_TYPE_PV] =
> + set_fds(d->master_fd[CONSOLE_TYPE_PV],
> + events|POLLPRI);
> + }
> +
> + if (d->vcon_enabled && d->master_fd[CONSOLE_TYPE_VCON] != -1) {
> + short events = 0;
> + if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_VCON))
> + events |= POLLIN;
> +
> + if (!buffer_empty(&d->buffer[CONSOLE_TYPE_VCON]))
> + events |= POLLOUT;
> +
> + if (events)
> + d->master_pollfd_idx[CONSOLE_TYPE_VCON] =
> + set_fds(d->master_fd[CONSOLE_TYPE_VCON],
> + events|POLLPRI);
> }
> }
>
> @@ -1166,6 +1346,16 @@ void handle_io(void)
>
> for (d = dom_head; d; d = n) {
> n = d->next;
> +
> + /*
> + * Check if the data pending flag is set for any of the consoles.
> + * If yes then service those first.
> + */
> + if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
> + buffer_append(d, CONSOLE_TYPE_PV);
> + else if ( d->console_data_pending & (1<<CONSOLE_TYPE_VCON) )
> + buffer_append(d, CONSOLE_TYPE_VCON);
> +
Why? You seem to have skipped the ratelimit check here.
> if (d->event_count < RATE_LIMIT_ALLOWANCE) {
> if (d->xce_handle != NULL &&
> d->xce_pollfd_idx != -1 &&
> @@ -1176,22 +1366,36 @@ void handle_io(void)
> handle_ring_read(d);
> }
>
> - if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
> - if (fds[d->master_pollfd_idx].revents &
> + if (d->master_fd[CONSOLE_TYPE_PV] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_PV] != -1) {
> + if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
> + ~(POLLIN|POLLOUT|POLLPRI))
> + domain_handle_broken_tty(d, domain_is_valid(d->domid));
> + else {
> + if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
> + POLLIN)
> + handle_tty_read(d, CONSOLE_TYPE_PV);
> + if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
> + POLLOUT)
> + handle_tty_write(d, CONSOLE_TYPE_PV);
> + }
> + }
> +
> + if (d->master_fd[CONSOLE_TYPE_VCON] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_VCON] != -1) {
> + if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
> ~(POLLIN|POLLOUT|POLLPRI))
> - domain_handle_broken_tty(d,
> - domain_is_valid(d->domid));
> + domain_handle_broken_tty(d, domain_is_valid(d->domid));
> else {
> - if (fds[d->master_pollfd_idx].revents &
> + if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
> POLLIN)
> - handle_tty_read(d);
> - if (fds[d->master_pollfd_idx].revents &
> + handle_tty_read(d, CONSOLE_TYPE_VCON);
> + if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
> POLLOUT)
> - handle_tty_write(d);
> + handle_tty_write(d, CONSOLE_TYPE_VCON);
> }
> }
>
> - d->xce_pollfd_idx = d->master_pollfd_idx = -1;
> + d->xce_pollfd_idx = d->master_pollfd_idx[CONSOLE_TYPE_PV] =
> + d->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
>
Indentation and line too long.
> if (d->last_seen != enum_pass)
> shutdown_domain(d);
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles
2017-04-12 16:33 ` Wei Liu
@ 2017-04-24 14:52 ` Bhupinder Thakur
0 siblings, 0 replies; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-24 14:52 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson
Hi Wei Liu,
Thanks for your comments.
On 12 April 2017 at 22:03, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Apr 03, 2017 at 03:14:31PM +0530, Bhupinder Thakur wrote:
>> Xenconsole supports only PV console currently. To get access to emulated pl011
>> uart another backend console is required.
>>
>> This patch modifies different data structures and APIs used
>> in xenconsole to support two console types: PV and VCON (virtual console).
>>
>> Change summary:
>>
>> 1. Modify the domain structure to support two console types: PV and a
>> virtual console (VCON).
>>
>> 2. Modify different APIs such as buffer_append() to take a new parameter
>> console_type as input and operate on the data structures indexed by
>> the console_type.
>>
>> 3. Modfications in domain_create_ring():
>> - Bind to the vpl011 event channel obtained from the xen store as a
>> new
>> parameter
>> - Map the PFN to its address space to be used as IN/OUT ring
>> buffers.
>> It obtains the PFN from the xen store as a new parameter
>>
>> 4. Modifications in handle_ring_read() to handle both PV and VCON
>> events.
>>
>> 5. Add a new log_file for VCON console logs.
>>
>
> It seems that this patch and previous one should be merged into one.
>
Yes I have merged this patch with the earlier one to maintain the bisectability.
> There are a lot of coding style issues, please fix all of them.
>
I will fix the coding style issues.
> The code looks rather repetitive unfortunately. I believe a lot of the
> repetitiveness can be avoided by using loops and pointers.
>
Stefano suggested to define an array of a console structure instead of
using different arrays for console related fields in the domain
structure.
I think that way the changes to the code will be much cleaner. I will
incorporate those changes.
>> static void domain_close_tty(struct domain *dom)
>> {
>> - if (dom->master_fd != -1) {
>> - close(dom->master_fd);
>> - dom->master_fd = -1;
>> + if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
>> + close(dom->master_fd[CONSOLE_TYPE_PV]);
>> + dom->master_fd[CONSOLE_TYPE_PV] = -1;
>> + }
>> +
>> + if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
>> + close(dom->slave_fd[CONSOLE_TYPE_PV]);
>> + dom->slave_fd[CONSOLE_TYPE_PV] = -1;
>> + }
>> +
>> + if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
>> + close(dom->master_fd[CONSOLE_TYPE_VCON]);
>> + dom->master_fd[CONSOLE_TYPE_VCON] = -1;
>> }
>>
>> - if (dom->slave_fd != -1) {
>> - close(dom->slave_fd);
>> - dom->slave_fd = -1;
>> + if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
>> + close(dom->slave_fd[CONSOLE_TYPE_VCON]);
>> + dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
>> }
>
> You can use two loops to avoid repetitive snippets. But I'm fine with
> the code as-is, too.
I have added a new function console_close_tty() which is called for
both pv and vuart consoles. The console
is abstracted as a separate structure.
static void console_close_tty(struct console *con)
{
if (con->master_fd != -1) {
close(con->master_fd);
con->master_fd = -1;
}
if (con->slave_fd != -1) {
close(con->slave_fd);
con->slave_fd = -1;
}
}
Other functions such as buffer_append(), handle_tty_read() will also
operate on the console structure.
>> @@ -1166,6 +1346,16 @@ void handle_io(void)
>>
>> for (d = dom_head; d; d = n) {
>> n = d->next;
>> +
>> + /*
>> + * Check if the data pending flag is set for any of the consoles.
>> + * If yes then service those first.
>> + */
>> + if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
>> + buffer_append(d, CONSOLE_TYPE_PV);
>> + else if ( d->console_data_pending & (1<<CONSOLE_TYPE_VCON) )
>> + buffer_append(d, CONSOLE_TYPE_VCON);
>> +
>
> Why? You seem to have skipped the ratelimit check here.
I have thought about this change and I think this check is not
required. The rate limit check will ensure that more data is not
appended to the buffer as it will keep the events masked and
handle_ring_read() will not be called. I will remove this check.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 09/10] xen/arm: vpl011: Add new virtual console to xenconsole client
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (7 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-19 18:55 ` Stefano Stabellini
2017-04-03 9:44 ` [PATCH 10/10] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
2017-04-20 12:47 ` [PATCH 00/10] pl011 emulation support in Xen Julien Grall
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
Add a new console type VCON to connect to the virtual console.
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/console/client/main.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 977779f..03b6fb1 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -264,6 +264,7 @@ typedef enum {
CONSOLE_INVAL,
CONSOLE_PV,
CONSOLE_SERIAL,
+ CONSOLE_VCON,
} console_type;
static struct termios stdin_old_attr;
@@ -361,6 +362,8 @@ int main(int argc, char **argv)
type = CONSOLE_SERIAL;
else if (!strcmp(optarg, "pv"))
type = CONSOLE_PV;
+ else if (!strcmp(optarg, "vcon"))
+ type = CONSOLE_VCON;
else {
fprintf(stderr, "Invalid type argument\n");
fprintf(stderr, "Console types supported are: serial, pv\n");
@@ -436,6 +439,9 @@ int main(int argc, char **argv)
else
snprintf(path, strlen(dom_path) + strlen("/device/console/%d/tty") + 5, "%s/device/console/%d/tty", dom_path, num);
}
+ if (type == CONSOLE_VCON) {
+ snprintf(path, strlen(dom_path) + strlen("/console/vtty") + 1, "%s/console/vtty", dom_path);
+ }
/* FIXME consoled currently does not assume domain-0 doesn't have a
console which is good when we break domain-0 up. To keep us
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 09/10] xen/arm: vpl011: Add new virtual console to xenconsole client
2017-04-03 9:44 ` [PATCH 09/10] xen/arm: vpl011: Add new virtual console to xenconsole client Bhupinder Thakur
@ 2017-04-19 18:55 ` Stefano Stabellini
0 siblings, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 18:55 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> Add a new console type VCON to connect to the virtual console.
VUART is a better name here too. Please add a doc under docs/misc to
document the new xenstore paths.
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> tools/console/client/main.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 977779f..03b6fb1 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -264,6 +264,7 @@ typedef enum {
> CONSOLE_INVAL,
> CONSOLE_PV,
> CONSOLE_SERIAL,
> + CONSOLE_VCON,
> } console_type;
>
> static struct termios stdin_old_attr;
> @@ -361,6 +362,8 @@ int main(int argc, char **argv)
> type = CONSOLE_SERIAL;
> else if (!strcmp(optarg, "pv"))
> type = CONSOLE_PV;
> + else if (!strcmp(optarg, "vcon"))
> + type = CONSOLE_VCON;
> else {
> fprintf(stderr, "Invalid type argument\n");
> fprintf(stderr, "Console types supported are: serial, pv\n");
> @@ -436,6 +439,9 @@ int main(int argc, char **argv)
> else
> snprintf(path, strlen(dom_path) + strlen("/device/console/%d/tty") + 5, "%s/device/console/%d/tty", dom_path, num);
> }
> + if (type == CONSOLE_VCON) {
> + snprintf(path, strlen(dom_path) + strlen("/console/vtty") + 1, "%s/console/vtty", dom_path);
> + }
>
> /* FIXME consoled currently does not assume domain-0 doesn't have a
> console which is good when we break domain-0 up. To keep us
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 10/10] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (8 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 09/10] xen/arm: vpl011: Add new virtual console to xenconsole client Bhupinder Thakur
@ 2017-04-03 9:44 ` Bhupinder Thakur
2017-04-19 18:53 ` Stefano Stabellini
2017-04-20 12:47 ` [PATCH 00/10] pl011 emulation support in Xen Julien Grall
10 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-03 9:44 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson
The SBSA uart node format is as specified in
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:
ARM SBSA defined generic UART
------------------------------
This UART uses a subset of the PL011 registers and consequently lives
in the PL011 driver. It's baudrate and other communication parameters
cannot be adjusted at runtime, so it lacks a clock specifier here.
Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier
- current-speed: the (fixed) baud rate set by the firmware
Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
tools/libxl/libxl_arm.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 46 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..b25fff6 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -57,6 +57,13 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
nr_spis = spi + 1;
}
+ /*
+ * If pl011 is enabled then increment the nr_spis to allow allocation
+ * of a SPI VIRQ for pl011.
+ */
+ if (libxl_defbool_val(d_config->b_info.enable_pl011))
+ nr_spis += 1;
+
LOG(DEBUG, "Configure the domain");
xc_config->nr_spis = nr_spis;
@@ -130,9 +137,10 @@ static struct arch_info {
const char *guest_type;
const char *timer_compat;
const char *cpu_compat;
+ const char *uart_compat;
} arch_info[] = {
- {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" },
- {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
+ {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
+ {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
};
/*
@@ -590,6 +598,39 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
return 0;
}
+static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
+ const struct arch_info *ainfo,
+ struct xc_dom_image *dom)
+{
+ int res;
+ gic_interrupt intr;
+
+ res = fdt_begin_node(fdt, "sbsa-pl011");
+ if (res) return res;
+
+ res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
+ if (res) return res;
+
+ res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+ 1,
+ GUEST_PL011_BASE, GUEST_PL011_SIZE);
+ if (res)
+ return res;
+
+ set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+
+ res = fdt_property_interrupts(gc, fdt, &intr, 1);
+ if (res) return res;
+
+ /* Use a default baud rate of 115200. */
+ fdt_property_u32(fdt, "current-speed", 115200);
+
+ res = fdt_end_node(fdt);
+ if (res) return res;
+
+ return 0;
+}
+
static const struct arch_info *get_arch_info(libxl__gc *gc,
const struct xc_dom_image *dom)
{
@@ -889,6 +930,9 @@ next_resize:
FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
FDT( make_hypervisor_node(gc, fdt, vers) );
+ if ( libxl_defbool_val(info->enable_pl011) )
+ FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
+
if (pfdt)
FDT( copy_partial_fdt(gc, fdt, pfdt) );
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 10/10] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree
2017-04-03 9:44 ` [PATCH 10/10] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
@ 2017-04-19 18:53 ` Stefano Stabellini
0 siblings, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-19 18:53 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Mon, 3 Apr 2017, Bhupinder Thakur wrote:
> The SBSA uart node format is as specified in
> Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:
>
> ARM SBSA defined generic UART
> ------------------------------
> This UART uses a subset of the PL011 registers and consequently lives
> in the PL011 driver. It's baudrate and other communication parameters
> cannot be adjusted at runtime, so it lacks a clock specifier here.
>
> Required properties:
> - compatible: must be "arm,sbsa-uart"
> - reg: exactly one register range
> - interrupts: exactly one interrupt specifier
> - current-speed: the (fixed) baud rate set by the firmware
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> tools/libxl/libxl_arm.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..b25fff6 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -57,6 +57,13 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> nr_spis = spi + 1;
> }
>
> + /*
> + * If pl011 is enabled then increment the nr_spis to allow allocation
> + * of a SPI VIRQ for pl011.
> + */
> + if (libxl_defbool_val(d_config->b_info.enable_pl011))
> + nr_spis += 1;
> +
> LOG(DEBUG, "Configure the domain");
>
> xc_config->nr_spis = nr_spis;
> @@ -130,9 +137,10 @@ static struct arch_info {
> const char *guest_type;
> const char *timer_compat;
> const char *cpu_compat;
> + const char *uart_compat;
> } arch_info[] = {
> - {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" },
> - {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
> + {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
> + {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
> };
>
> /*
> @@ -590,6 +598,39 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
> return 0;
> }
>
> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
> + const struct arch_info *ainfo,
> + struct xc_dom_image *dom)
> +{
> + int res;
> + gic_interrupt intr;
> +
> + res = fdt_begin_node(fdt, "sbsa-pl011");
> + if (res) return res;
> +
> + res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
> + if (res) return res;
> +
> + res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> + 1,
> + GUEST_PL011_BASE, GUEST_PL011_SIZE);
strange indentation
> + if (res)
> + return res;
you might as well keep doing if (res) return res; everywhere.
Aside from these two minor issues:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> + set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> + res = fdt_property_interrupts(gc, fdt, &intr, 1);
> + if (res) return res;
> +
> + /* Use a default baud rate of 115200. */
> + fdt_property_u32(fdt, "current-speed", 115200);
> +
> + res = fdt_end_node(fdt);
> + if (res) return res;
> +
> + return 0;
> +}
> +
> static const struct arch_info *get_arch_info(libxl__gc *gc,
> const struct xc_dom_image *dom)
> {
> @@ -889,6 +930,9 @@ next_resize:
> FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
> FDT( make_hypervisor_node(gc, fdt, vers) );
>
> + if ( libxl_defbool_val(info->enable_pl011) )
> + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> +
> if (pfdt)
> FDT( copy_partial_fdt(gc, fdt, pfdt) );
>
> --
> 2.7.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 00/10] pl011 emulation support in Xen
2017-04-03 9:44 [PATCH 00/10] pl011 emulation support in Xen Bhupinder Thakur
` (9 preceding siblings ...)
2017-04-03 9:44 ` [PATCH 10/10] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
@ 2017-04-20 12:47 ` Julien Grall
2017-04-26 15:21 ` Bhupinder Thakur
10 siblings, 1 reply; 53+ messages in thread
From: Julien Grall @ 2017-04-20 12:47 UTC (permalink / raw)
To: Bhupinder Thakur, xen-devel; +Cc: Wei Liu, Stefano Stabellini, Ian Jackson
Hi Bhupinder,
On 03/04/17 10:44, Bhupinder Thakur wrote:
> tools/console/client/main.c | 6 +
> tools/console/daemon/io.c | 532 +++++++++++++++++++++++++++------------
> tools/libxc/include/xc_dom.h | 3 +
> tools/libxc/xc_dom_arm.c | 7 +-
> tools/libxc/xc_dom_boot.c | 3 +
> tools/libxc/xc_domain.c | 7 +
> tools/libxl/libxl.c | 10 +
> tools/libxl/libxl_arm.c | 48 +++-
> tools/libxl/libxl_create.c | 12 +
> tools/libxl/libxl_dom.c | 13 +-
> tools/libxl/libxl_internal.h | 5 +
> tools/libxl/libxl_types.idl | 2 +
> tools/libxl/xl_cmdimpl.c | 4 +
> xen/arch/arm/Kconfig | 5 +
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/domain.c | 11 +
> xen/arch/arm/hvm.c | 112 +++++++++
> xen/arch/arm/vpl011.c | 339 +++++++++++++++++++++++++
> xen/common/domctl.c | 3 +
> xen/include/asm-arm/domain.h | 5 +
> xen/include/asm-arm/pl011-uart.h | 2 +
> xen/include/public/arch-arm.h | 8 +
> xen/include/public/domctl.h | 2 +
> xen/include/public/hvm/params.h | 10 +
> xen/include/xen/sched.h | 4 +
> xen/include/xen/vpl011.h | 67 +++++
I was expecting an update of the documentation for the new options
introduced and the expected behavior for IRQ numbering when using both
pl011 and assign physical IRQ.
Please add the documentation in the next version.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 00/10] pl011 emulation support in Xen
2017-04-20 12:47 ` [PATCH 00/10] pl011 emulation support in Xen Julien Grall
@ 2017-04-26 15:21 ` Bhupinder Thakur
2017-04-26 17:09 ` Stefano Stabellini
0 siblings, 1 reply; 53+ messages in thread
From: Bhupinder Thakur @ 2017-04-26 15:21 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu
Hi Julien,
>> tools/console/client/main.c | 6 +
>> tools/console/daemon/io.c | 532
>> +++++++++++++++++++++++++++------------
>> tools/libxc/include/xc_dom.h | 3 +
>> tools/libxc/xc_dom_arm.c | 7 +-
>> tools/libxc/xc_dom_boot.c | 3 +
>> tools/libxc/xc_domain.c | 7 +
>> tools/libxl/libxl.c | 10 +
>> tools/libxl/libxl_arm.c | 48 +++-
>> tools/libxl/libxl_create.c | 12 +
>> tools/libxl/libxl_dom.c | 13 +-
>> tools/libxl/libxl_internal.h | 5 +
>> tools/libxl/libxl_types.idl | 2 +
>> tools/libxl/xl_cmdimpl.c | 4 +
>> xen/arch/arm/Kconfig | 5 +
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/domain.c | 11 +
>> xen/arch/arm/hvm.c | 112 +++++++++
>> xen/arch/arm/vpl011.c | 339 +++++++++++++++++++++++++
>> xen/common/domctl.c | 3 +
>> xen/include/asm-arm/domain.h | 5 +
>> xen/include/asm-arm/pl011-uart.h | 2 +
>> xen/include/public/arch-arm.h | 8 +
>> xen/include/public/domctl.h | 2 +
>> xen/include/public/hvm/params.h | 10 +
>> xen/include/xen/sched.h | 4 +
>> xen/include/xen/vpl011.h | 67 +++++
>
>
> I was expecting an update of the documentation for the new options
> introduced and the expected behavior for IRQ numbering when using both pl011
> and assign physical IRQ.
>
> Please add the documentation in the next version.
>
I have added details on the vuart console option in docs/misc/console.txt.
I have added a check that if user tries to allocate a physical IRQ
which is same as the pl011 SPI then domain creation would fail. Is
there any IRQ specific document where I can add this behavior?
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 00/10] pl011 emulation support in Xen
2017-04-26 15:21 ` Bhupinder Thakur
@ 2017-04-26 17:09 ` Stefano Stabellini
0 siblings, 0 replies; 53+ messages in thread
From: Stefano Stabellini @ 2017-04-26 17:09 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu
On Wed, 26 Apr 2017, Bhupinder Thakur wrote:
> Hi Julien,
>
>
> >> tools/console/client/main.c | 6 +
> >> tools/console/daemon/io.c | 532
> >> +++++++++++++++++++++++++++------------
> >> tools/libxc/include/xc_dom.h | 3 +
> >> tools/libxc/xc_dom_arm.c | 7 +-
> >> tools/libxc/xc_dom_boot.c | 3 +
> >> tools/libxc/xc_domain.c | 7 +
> >> tools/libxl/libxl.c | 10 +
> >> tools/libxl/libxl_arm.c | 48 +++-
> >> tools/libxl/libxl_create.c | 12 +
> >> tools/libxl/libxl_dom.c | 13 +-
> >> tools/libxl/libxl_internal.h | 5 +
> >> tools/libxl/libxl_types.idl | 2 +
> >> tools/libxl/xl_cmdimpl.c | 4 +
> >> xen/arch/arm/Kconfig | 5 +
> >> xen/arch/arm/Makefile | 1 +
> >> xen/arch/arm/domain.c | 11 +
> >> xen/arch/arm/hvm.c | 112 +++++++++
> >> xen/arch/arm/vpl011.c | 339 +++++++++++++++++++++++++
> >> xen/common/domctl.c | 3 +
> >> xen/include/asm-arm/domain.h | 5 +
> >> xen/include/asm-arm/pl011-uart.h | 2 +
> >> xen/include/public/arch-arm.h | 8 +
> >> xen/include/public/domctl.h | 2 +
> >> xen/include/public/hvm/params.h | 10 +
> >> xen/include/xen/sched.h | 4 +
> >> xen/include/xen/vpl011.h | 67 +++++
> >
> >
> > I was expecting an update of the documentation for the new options
> > introduced and the expected behavior for IRQ numbering when using both pl011
> > and assign physical IRQ.
> >
> > Please add the documentation in the next version.
> >
> I have added details on the vuart console option in docs/misc/console.txt.
>
> I have added a check that if user tries to allocate a physical IRQ
> which is same as the pl011 SPI then domain creation would fail. Is
> there any IRQ specific document where I can add this behavior?
I don't think so. Maybe you could add an line about this to the xl.cfg
man page under the irqs= section?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 53+ messages in thread