Hi, On 07/06/17 00:02, Stefano Stabellini wrote:
On Tue, 6 Jun 2017, Bhupinder Thakur wrote:+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;After reading the indexes, we always need barriers. In this case: smp_rmb();
Well, there are already barrier with the spinlock. However, I am bit concern about reading those index without the lock taken. You can have concurrent call to vpl011_read_data happening and therefore the indexes may have changed when the lock will be taken.
+ 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(); + } + else + { + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); + } + + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) + { + vpl011->uartfr |= RXFE; + vpl011->uartris &= ~RXI; + } + vpl011->uartfr &= ~RXFF; + VPL011_UNLOCK(d, flags);I am pretty sure that the PV console protocol requires us to notify the other end even on reads. We need to add a notify_via_xen_event_channel here, I think.
I would agree here. On the previous version, I asked Bhupinder to explain why it is necessary and he said: "On second thoughs, notification is not required".
+ return data; +} + +static void vpl011_write_data(struct domain *d, uint8_t data) +{ + unsigned long flags; + struct vpl011 *vpl011 = &d->arch.vpl011; + struct xencons_interface *intf = vpl011->ring_buf; + XENCONS_RING_IDX out_cons = intf->out_cons; + XENCONS_RING_IDX out_prod = intf->out_prod;smp_mb()
Same remark as above. [...]
+static void vpl011_data_avail(struct domain *d) +{ + unsigned long flags; + 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; + XENCONS_RING_IDX out_cons = intf->out_cons; + XENCONS_RING_IDX out_prod = intf->out_prod; + XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;smb_mb()
Ditto. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
