Hi Bhupinder,
On 21/02/17 11:25, Bhupinder Thakur wrote:
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
new file mode 100644
index 0000000..88ba968
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
[...]
+static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r,
void *priv)
See my comment on the vpl011_mmio_read about the structure of the function.
+{
+ unsigned char ch = r;
+
+ switch (info->gpa - GUEST_PL011_BASE)
+ {
+ case VPL011_UARTCR_OFFSET:
This register is not required in the SBSA UART.
+ v->domain->arch.vpl011.control = r;
+ break;
+ case VPL011_UARTDR_OFFSET:
+ vpl011_write_data(v->domain, ch);
The implicit casting of register_t to ch is a bit confusing. Also I
would prefer to use uint8_t as it reflects the size of the field.
Lastly, what if vpl011_write_data is returning an error?
+ break;
+ case VPL011_UARTIMSC_OFFSET:
+ v->domain->arch.vpl011.intr_mask = r;
You need to sanitize the value written and make sure reserved field and
Write As Ones register and handle correctly (see the spec).
+ if ( (v->domain->arch.vpl011.raw_intr_status &
v->domain->arch.vpl011.intr_mask) )
This line and the line below are over 80 columns. Also the code in
general would benefits if you define a local variable to point to
v->domain->arch.vpl011.
+ vgic_vcpu_inject_spi(v->domain,
(int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
I don't think we need an HVM_PARAM for the pl011 SPI. We cannot hardcode
this in the guest layout (see include/public/arch-arm.h).
+ break;
+ case VPL011_UARTICR_OFFSET:
+ /*
+ * clear all bits which are set in the input
+ */
+ v->domain->arch.vpl011.raw_intr_status &= ~r;
+ if ( (v->domain->arch.vpl011.raw_intr_status &
v->domain->arch.vpl011.intr_mask) )
+ {
{ } are not necessary.
+ vgic_vcpu_inject_spi(v->domain,
(int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
+ }
The if is exactly the same as the on in IMSC. This is the call of an
helper to check the interrupt state and inject one if necessary.
+ break;
+ case VPL011_UARTRSR_OFFSET: // nothing to clear
The coding style for single line comment in Xen is /* ... */
Also, I think we may want to handle Overrun error as the ring may be full.
+ break;
+ case VPL011_UARTFR_OFFSET: // these are all RO registers
+ case VPL011_UARTRIS_OFFSET:
+ case VPL011_UARTMIS_OFFSET:
+ case VPL011_UARTDMACR_OFFSET:
Please have a look at the vGICv2/vGICv3 emulation see how we implemented
write ignore register.
+ break;
+ default:
+ printk ("vpl011_mmio_write: switch case not handled %d\n",
(int)(info->gpa - GUEST_PL011_BASE));
See my comment about the prinkt in the read emulation.
+ break;
+ }
+
+ return VPL011_EMUL_OK;
+}
+
+static const struct mmio_handler_ops vpl011_mmio_handler = {
+ .read = vpl011_mmio_read,
+ .write = vpl011_mmio_write,
+};
+
+
+
Only newline is sufficient. This was mention by Konrad and I will try to
avoid repeating his comments.
+int vpl011_map_guest_page(struct domain *d)
+{
+ int rc=0;
+
+ /*
+ * map the guest PFN to Xen address space
+ */
+ rc = prepare_ring_for_helper(d,
+
d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN],
+ &d->arch.vpl011.ring_page,
+ (void **)&d->arch.vpl011.ring_buf);
Why do you need the cast?
Also I cannot find any code in this series which destroy the ring.
Please have a helper called vpl011_unmap_guest_page to do this job and
call when the domain is destroyed.
+ if ( rc < 0 )
+ {
+ printk("Failed to map vpl011 guest PFN\n");
+ }
+
+ return rc;
+}
+
+static int vpl011_data_avail(struct domain *d)
+{
+ int rc=0;
+ unsigned long flags;
+
+ struct console_interface *intf=(struct console_interface
*)d->arch.vpl011.ring_buf;
+
+ VPL011_LOCK(d, flags);
+
+ /*`
+ * check IN ring buffer
+ */
+ if ( !VPL011_IN_RING_EMPTY(intf) )
+ {
+ /*
+ * clear the RX FIFO empty flag as the ring is not empty
+ */
+ d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE);
+
+ /*
+ * if the buffer is full then set the RX FIFO FULL flag
+ */
+ if ( VPL011_IN_RING_FULL(intf) )
+ d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF);
+
+ /*
+ * set the RX interrupt status
+ */
+ d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS);
+ }
+
+ /*
+ * check OUT ring buffer
+ */
+ if ( !VPL011_OUT_RING_FULL(intf) )
+ {
+ /*
+ * if the buffer is not full then clear the TX FIFO full flag
+ */
+ d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF);
+
+ /*
+ * set the TX interrupt status
+ */
+ d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS);
+
+ if ( VPL011_OUT_RING_EMPTY(intf) )
+ {
+ /*
+ * clear the uart busy flag and set the TX FIFO empty flag
+ */
+ d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY);
+ d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE);
+ }
+ }
+
+ VPL011_UNLOCK(d, flags);
+
+ /*
+ * send an interrupt if it is not masked
+ */
+ if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) )
+ vgic_vcpu_inject_spi(d,
(int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
See my comment above about having an helper for this code.
+
+ if ( !VPL011_OUT_RING_EMPTY(intf) )
+ {
+ /*
+ * raise an interrupt to dom0
The backend console does not necessarily live in DOM0. Anyway, Konrad
requested to drop the comment and I am happy with that.
+ */
+ rc = raw_evtchn_send(d,
+
d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL);
You are using a function that is been defined in a later patch (i.w #3).
All the patch should be able to compile without applying upcoming patch
to allow bisection.
Ideally, Xen should still be functional for every patch. But it is a
best effort.
+
+ if ( rc < 0 )
+ printk("Failed to send vpl011 interrupt to dom0\n");
+ }
+
+ return rc;
+}
+
+int vpl011_read_data(struct domain *d, unsigned char *data)
This function does not seem to be used outside of this file. So why do
you export it?
+{
+ int rc=0;
+ unsigned long flags;
+ struct console_interface *intf=(struct console_interface
*)d->arch.vpl011.ring_buf;
+
+ *data = 0;
+
+ VPL011_LOCK(d, flags);
+
+ /*
+ * if there is data in the ring buffer then copy it to the output buffer
+ */
+ if ( !VPL011_IN_RING_EMPTY(intf) )
+ {
+ *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)];
+ }
+
+ /*
+ * if the ring buffer is empty then set the RX FIFO empty flag
+ */
+ if ( VPL011_IN_RING_EMPTY(intf) )
+ {
+ d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE);
+ d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS);
+ }
+
+ /*
+ * clear the RX FIFO full flag
+ */
+ d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF);
+
+ VPL011_UNLOCK(d, flags);
+
+ return rc;
+}
+
+int vpl011_write_data(struct domain *d, unsigned char data)
Same has for vpl011_read_data.
+{
+ int rc=0;
+ unsigned long flags;
+ struct console_interface *intf=(struct console_interface
*)d->arch.vpl011.ring_buf;
+
+ VPL011_LOCK(d, flags);
+
+ /*
+ * if there is space in the ring buffer then write the data
+ */
+ if ( !VPL011_OUT_RING_FULL(intf) )
+ {
+ intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data;
+ smp_wmb();
+ }
+
+ /*
+ * if there is no space in the ring buffer then set the
+ * TX FIFO FULL flag
+ */
+ if ( VPL011_OUT_RING_FULL(intf) )
+ {
+ d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF);
+ d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS);
+ }
+
+ /*
+ * set the uart busy status
+ */
+ d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY);
+
+ /*
+ * clear the TX FIFO empty flag
+ */
+ d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE);
+
+ VPL011_UNLOCK(d, flags);
+
+ /*
+ * raise an event to dom0 only if it is the first character in the buffer
+ */
+ if ( VPL011_RING_DEPTH(intf, out) == 1 )
+ {
+ rc = raw_evtchn_send(d,
+ d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN],
NULL);
+
+ if ( rc < 0 )
+ printk("Failed to send vpl011 interrupt to dom0\n");
+ }
+
+ return rc;
+}
+
+static void vpl011_notification(struct vcpu *v, unsigned int port)
+{
+ vpl011_data_avail(v->domain);
vpl011_data_avail is returning an error but you don't check. What is the
point of it then?
+}
+
+int domain_vpl011_init(struct domain *d)
I was expected a destroy function to clean-up the vpl011 emulation.
+{
+ int rc=0;
+
+ /*
+ * register xen event channel
+ */
+ rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id,
+ vpl011_notification);
This line looks wrong to me. The console backend may not live in the
same domain as the toolstack.
+ if (rc < 0)
+ {
+ printk ("Failed to allocate vpl011 event channel\n");
+ return rc;
+ }
+ d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc;
+
+ /*
+ * allocate an SPI VIRQ for the guest
+ */
+ d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d);
It makes little sense to hardcode the MMIO region and not the SPI. Also,
I would rather avoid to introduce new HVM_PARAM when not necessary. So
hardcoding the value looks more sensible to me.
+
+ /*
+ * register mmio handler
+ */
+ register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE,
GUEST_PL011_SIZE, NULL);
Coding style. No space between the function name and (.
+
+ /*
+ * initialize the lock
+ */
+ spin_lock_init(&d->arch.vpl011.lock);
+
+ /*
+ * clear the flag, control and interrupt status registers
+ */
+ d->arch.vpl011.control = 0;
+ d->arch.vpl011.flag = 0;
+ d->arch.vpl011.intr_mask = 0;
+ d->arch.vpl011.intr_clear = 0;
+ d->arch.vpl011.raw_intr_status = 0;
+ d->arch.vpl011.masked_intr_status = 0;
The domain structure is already zeroed. So no need to 0 it again.
+
+ return 0;
+}
Missing e-macs magic here.
diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h
new file mode 100644
index 0000000..f2c577f
--- /dev/null
+++ b/xen/arch/arm/vpl011.h
Header should live in include.
[...]
+#ifndef _VPL011_H_
+
+#define _VPL011_H_
+
+/*
+ * register offsets
+ */
We already define the PL011 register in include/asm-arm/pl011-uart.h.
Please re-use them rather than re-inventing the wheel.
+#define VPL011_UARTDR_OFFSET 0x0 // data register (RW)
+#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear register
(RW)
+#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO)
+#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register (RO)
+#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register (RO)
+#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register (RW)
+#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO)
+#define VPL011_UARTCR_OFFSET 0x30 // uart control register
+#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register
[...]
+#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1))
+struct console_interface {
+ char in[1024];
+ char out[2048];
+ uint32_t in_cons, in_prod;
+ uint32_t out_cons, out_prod;
+};
+#endif
The communication between Xen and the console backend is based on the PV
console ring. It would be easier to re-use it rather than recreating one.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f2ecbc4..7e2feac 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP
The only user of this is Live patching.
If unsure, say Y.
+
+config VPL011_CONSOLE
+ bool "Emulated pl011 console support"
+ default y
+ ---help---
+ Allows a guest to use pl011 UART as a console
endmenu
This should go in arch/arm/Kconfig
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2d6fbb1..ff2403a 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -40,6 +40,7 @@ struct vtimer {
uint64_t cval;
};
+
Spurious line.
struct arch_domain
{
#ifdef CONFIG_ARM_64
@@ -131,6 +132,20 @@ struct arch_domain
struct {
uint8_t privileged_call_enabled : 1;
} monitor;
+
+#ifdef CONFIG_VPL011_CONSOLE
+ struct vpl011 {
+ void *ring_buf;
+ struct page_info *ring_page;
+ uint32_t flag; /* flag register */
+ uint32_t control; /* control register */
+ uint32_t intr_mask; /* interrupt mask register*/
+ uint32_t intr_clear; /* interrupt clear register */
+ uint32_t raw_intr_status; /* raw interrupt status register */
+ uint32_t masked_intr_status; /* masked interrupt register */
+ spinlock_t lock;
+ } vpl011;
+#endif
I think the structure should be defined in vpl011.h.
} __cacheline_aligned;
struct arch_vcpu
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index bd974fb..1d4548f 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -410,6 +410,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.
@@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t;
#define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000)
#define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000)
+
Spurious line.
#define GUEST_RAM_BANKS 2
#define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
