[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen
On 13/06/17 11:57, Bhupinder Thakur wrote:
Hi Julien,
Hi Bhupinder,
tools/console/daemon/io.c | 2 +-
xen/arch/arm/Kconfig | 5 +
xen/arch/arm/Makefile | 1 +
xen/arch/arm/vpl011.c | 418
+++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/domain.h | 6 +
xen/include/asm-arm/pl011-uart.h | 2 +
xen/include/asm-arm/vpl011.h | 74 +++++++
xen/include/public/arch-arm.h | 6 +
xen/include/public/io/console.h | 4 +
This would require an ACK from Konrad. The addition would also need to be
justified in the commit message. Although, you probably want to split this
change in a separate patch.
I will send this change in a separate patch.
9 files changed, 517 insertions(+), 1 deletion(-)
create mode 100644 xen/arch/arm/vpl011.c
create mode 100644 xen/include/asm-arm/vpl011.h
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..947f13a 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
Can you explain why you change the position of the include in io.c?
Since I am including ring.h in console.h, it needs string.h to be
included first.
This should be justified in the commit message.
[...]
+}
+
+static void vpl011_update(struct domain *d)
+{
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+
+ /*
+ * TODO: PL011 interrupts are level triggered which means
+ * that interrupt needs to be set/clear instead of being
+ * injected. However, currently vGIC does not handle level
+ * triggered interrupts properly. This function needs to be
+ * revisited once vGIC starts handling level triggered
+ * interrupts.
+ */
+ if ( vpl011->uartris & vpl011->uartimsc )
The write in uartirs and uartimsc are protected by a lock. Shouldn't it be
the case here too? More that they are not updated atomically.
You probably want to call vpl011_update with vpl011 lock taken to make sure
you don't have any synchronization issue.
Since we are just reading the values here, I think it is fine to not
take a lock. The
condition will either be true or false.
uartris and uartimsc may not be updated atomically because
vreg_reg32_update does not guarantee it.
So you may read a wrong value here and potentially inject spurious
interrupt. This will get much worse when level will fully be supported
as you may switch the level of the interrupt by mistake.
+ vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
+}
+
+static uint8_t vpl011_read_data(struct domain *d)
+{
+ unsigned long flags;
+ uint8_t data = 0;
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+ struct xencons_interface *intf = vpl011->ring_buf;
+ XENCONS_RING_IDX in_cons = intf->in_cons;
+ XENCONS_RING_IDX in_prod = intf->in_prod;
See my answer on Stefano's e-mail regarding the barrier here.
(<fa3e5003-5c7f-0886-d437-6b643347b4c5@xxxxxxx>)
+
+ 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 when TXFE bit is set then 0 will be
returned.
+ */
+ if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
+ {
+ data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
+ in_cons += 1;
+ intf->in_cons = in_cons;
+ smp_mb();
I don't understand why you moved the barrier from between reading the data
and intf->in_cons. You have to ensure the character is read before updating
in_cons.
I thought that since these 3 statements are dependent on in_cons, they
would be executed in order due to data dependency.
How do you know the compiler will generate assembly which contain the
data dependency?
Likely it will not be the case because in_cons will be used indirectly
as we mask it first.
The memory barrier
after the 3 statements ensures that intf->in_cons is updated before
proceeding ahead.
Can you explain why?
IHMO, what matter here is in_cons to be written after intf->in[...] is
read. Otherwise the backend may see in_cons before the character has
effectively been read.
What if the other end of the ring has put more data whilst reading one
character?
It will raise an event when the other end puts more data and in the
event handling function data_available(), it will clear the RXFE bit.
And this is fine because the lock is here to protect uartfr/uartis I guess?
[...]
+
+ /* 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 = vgic_reserve_virq(d, GUEST_VPL011_SPI);
+ if ( !rc )
+ {
+ rc = -EINVAL;
+ goto out1;
+ }
+
+ register_mmio_handler(d, &vpl011_mmio_handler,
+ GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
Again, you register MMIO handler but never remove them. So if this call
fail, you will end up with the handlers existing but the rest
half-initialized which likely lead to a segfault, or worst leaking data.
This function does not return a status. So there is no way to find out
if the mmio handlers were
registered successfully.
That's not my point. register_mmio_handler should never fail. However,
the code below (alloc_unbount_...) may fail. And you bail
I am removing the mmio handlers in the error
legs in domain_vpl011_init().
Xen does not have any helper to revert the behavior of
register_mmio_handler and I don't seem to introduce why. So how do you
do it?
Anyway, you will not need to worry about removing MMIO handler if you
move the call after all the call that may fail.
+
+ spin_lock_init(&vpl011->lock);
+
+ rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
+ vpl011_notification);
+ if ( rc < 0 )
You
I think this is a type.
hmmm likely.
+ goto out2;
+
+ vpl011->evtchn = info->evtchn = rc;
+
+ return 0;
+
+out2:
+ xfree(d->arch.vmmio.handlers);
+ vgic_free_virq(d, GUEST_VPL011_SPI);
+
+out1:
+ destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+
+out:
+ return rc;
+}
+
+void domain_vpl011_deinit(struct domain *d)
+{
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+
+ if ( !vpl011->ring_buf )
You will bail out if ring_buf is NULL. However, if you called
domain_vpl011_init first and it failed, you may have ring_buf set but the
rest not fully updated. This means that you will free garbagge.
I think this could be solved by reinitialize ring_buf if an error occur in
domain_vpl011_init.
destroy_ring_for_helper() sets the first parameter to NULL incase it fails.
Fine. I think it is a bit fragile, but I don't see why someone would
decide to remove it without checking all the callers.
[...]
+#ifdef CONFIG_VPL011_CONSOLE
+int domain_vpl011_init(struct domain *d,
+ struct vpl011_init_info *info);
+void domain_vpl011_deinit(struct domain *d);
+#else
+static inline int domain_vpl011_init(struct domain *d,
+ struct vpl011_init_info *info)
+{
+ return -ENOSYS;
+}
+
+static inline void domain_vpl011_deinit(struct domain *d) { }
+#endif
+
+#endif
+
Please drop this newline.
You mean the newline between the #endifs or after the last #endif?
Yes.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
