Hi Stefano,
On 06/13/2018 11:15 PM, Stefano Stabellini wrote:
Make vpl011 being able to be used without a userspace component in Dom0.
In that case, output is printed to the Xen serial and input is received
from the Xen serial one character at a time.
Call domain_vpl011_init during construct_domU.
Signed-off-by: Stefano Stabellini <stefa...@xilinx.com>
---
xen/arch/arm/domain_build.c | 9 +++-
xen/arch/arm/vpl011.c | 98 +++++++++++++++++++++++++++++++++-----------
xen/include/asm-arm/vpl011.h | 2 +
3 files changed, 84 insertions(+), 25 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ff65057..97f14ca 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2482,7 +2482,14 @@ int __init construct_domU(struct domain *d, struct
dt_device_node *node)
if ( rc < 0 )
return rc;
- return __construct_domain(d, &kinfo);
+ rc = __construct_domain(d, &kinfo);
+ if ( rc < 0 )
+ return rc;
+
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+ rc = domain_vpl011_init(d, NULL);
See my remark on the previous patch about exposing vpl011 by default.
+#endif
+ return rc;
}
int __init construct_dom0(struct domain *d)
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index a281eab..5f1dc7a 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -34,6 +34,8 @@
#include <asm/vgic-emul.h>
#include <asm/vpl011.h>
+static void vpl011_data_avail(struct domain *d);
+
/*
* Since pl011 registers are 32-bit registers, all registers
* are handled similarly allowing 8-bit, 16-bit and 32-bit
@@ -77,6 +79,29 @@ static void vpl011_update_interrupt_status(struct domain *d)
#endif
}
+void vpl011_read_char(struct domain *d, char c)
The name is slightly odd. From the name, I would expect that a character
is returned. But in fact, you write a character you received in the
ring. So a better name would be vpl011_rx_char.
+{
+ unsigned long flags;
+ XENCONS_RING_IDX in_cons, in_prod;
+ struct xencons_interface *intf = d->arch.vpl011.ring_buf;
+
+ VPL011_LOCK(d, flags);
+
+ in_cons = intf->in_cons;
+ in_prod = intf->in_prod;
+ if (xencons_queued(in_prod, in_cons, sizeof(intf->in)) == sizeof(intf->in))
+ {
+ VPL011_UNLOCK(d, flags);
+ return;
+ }
+
+ intf->in[xencons_mask(in_prod, sizeof(intf->in))] = c;
+ intf->in_prod = in_prod + 1;
+
+ VPL011_UNLOCK(d, flags);
+ vpl011_data_avail(d);
+}
+
static uint8_t vpl011_read_data(struct domain *d)
{
unsigned long flags;
@@ -166,9 +191,18 @@ static void vpl011_write_data(struct domain *d, uint8_t
data)
struct vpl011 *vpl011 = &d->arch.vpl011;
struct xencons_interface *intf = vpl011->ring_buf;
XENCONS_RING_IDX out_cons, out_prod;
+ unsigned int fifo_level = 0;
VPL011_LOCK(d, flags);
+ if ( vpl011->ring_page == NULL )
+ {
+ printk("%c", data);
+ if (data == '\n')
+ printk("DOM%u: ", d->domain_id);
+ goto done;
+ }
+
I would rather introduce separate function to read/write data for the
case without PV console. And use it where appropriate. This would make
the code slightly easier to understand because "ring_page == NULL" is
slightly untuitive.
An idea would be introduce callback and set them during the
initialization of the vpl011 for the domain.
out_cons = intf->out_cons;
out_prod = intf->out_prod;
@@ -184,13 +218,10 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
sizeof (intf->out) )
{
- unsigned int fifo_level;
-
intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
out_prod += 1;
smp_wmb();
intf->out_prod = out_prod;
-
Spurious change.
fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));
if ( fifo_level == sizeof(intf->out) )
@@ -205,14 +236,15 @@ static void vpl011_write_data(struct domain *d, uint8_t
data)
*/
vpl011->uartfr |= BUSY;
}
-
- vpl011_update_tx_fifo_status(vpl011, fifo_level);
-
- vpl011_update_interrupt_status(d);
}
else
gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
+done:
+ vpl011_update_tx_fifo_status(vpl011, fifo_level);
+
+ vpl011_update_interrupt_status(d);
Hmmm, now you will call vpl011_update_* in the error case when writing
to the case. If you want to keep that, this should at least be explained
in the commit message or probably be a separate patch.
+
vpl011->uartfr &= ~TXFE;
VPL011_UNLOCK(d, flags);
@@ -462,13 +494,30 @@ int domain_vpl011_init(struct domain *d, struct
vpl011_init_info *info)
if ( vpl011->ring_buf )
return -EINVAL;
- /* Map the guest PFN to Xen address space. */
- rc = prepare_ring_for_helper(d,
- gfn_x(info->gfn),
- &vpl011->ring_page,
- &vpl011->ring_buf);
- if ( rc < 0 )
- goto out;
+ if ( info != NULL )
Please document how info could be NULL here.
+ {
+ /* Map the guest PFN to Xen address space. */
+ rc = prepare_ring_for_helper(d,
+ gfn_x(info->gfn),
+ &vpl011->ring_page,
+ &vpl011->ring_buf);
+ if ( rc < 0 )
+ goto out;
+
+ rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
+ vpl011_notification);
+ if ( rc < 0 )
+ goto out2;
When you move code around, you should also look at the error path. In
that case, you are going to free a virq that does not exist (because not
yet allocated), and if vgic_reserve_virq below fails, you will not free
the event channel allocated.
+
+ vpl011->evtchn = info->evtchn = rc;
+ } else {
+ vpl011->ring_buf = xzalloc(struct xencons_interface);
Using ring_buf is such waste of memory. You basically only allow 1024
character but still using 4K.
Furthermore, the way you use ring_page is really confusing. A bit more
documentation might help. Although, this new code does not feel
integrated with the rest of the vpl011.
It looks like you want to rework the vpl011 code to have move anything
related to ring in separate function. Once it is done, we could then
introduce new callback for the guest started in Xen.
+ if ( vpl011->ring_buf == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+ }
rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
if ( !rc )
@@ -477,13 +526,6 @@ int domain_vpl011_init(struct domain *d, struct
vpl011_init_info *info)
goto out1;
}
- rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
- vpl011_notification);
- if ( rc < 0 )
- goto out2;
-
- vpl011->evtchn = info->evtchn = rc;
-
spin_lock_init(&vpl011->lock);
register_mmio_handler(d, &vpl011_mmio_handler,
@@ -495,7 +537,10 @@ out2:
vgic_free_virq(d, GUEST_VPL011_SPI);
out1:
- destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+ if ( vpl011->ring_page == NULL && vpl011->ring_buf != NULL )
+ xfree(vpl011->ring_buf);
+ else
+ destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
out:
return rc;
@@ -508,8 +553,13 @@ void domain_vpl011_deinit(struct domain *d)
if ( !vpl011->ring_buf )
return;
- free_xen_event_channel(d, vpl011->evtchn);
- destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+ if ( vpl011->ring_page == NULL && vpl011->ring_buf != NULL )
+ {
+ xfree(vpl011->ring_buf);
+ } else {
Coding style
}
else
{
+ free_xen_event_channel(d, vpl011->evtchn);
+ destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+ }
}
/*
diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
index db95ff8..8d9b0da 100644
--- a/xen/include/asm-arm/vpl011.h
+++ b/xen/include/asm-arm/vpl011.h
@@ -53,6 +53,7 @@ struct vpl011_init_info {
int domain_vpl011_init(struct domain *d,
struct vpl011_init_info *info);
void domain_vpl011_deinit(struct domain *d);
+void vpl011_read_char(struct domain *d, char c);
#else
static inline int domain_vpl011_init(struct domain *d,
struct vpl011_init_info *info)
@@ -61,6 +62,7 @@ static inline int domain_vpl011_init(struct domain *d,
}
static inline void domain_vpl011_deinit(struct domain *d) { }
+static inline void vpl011_read_char(struct domain *d, char c) { }
#endif
#endif /* _VPL011_H_ */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
