|
[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
Hi Julien,
>>
>> 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.
>
>> @@ -21,6 +21,7 @@
>>
>> #include "utils.h"
>> #include "io.h"
>> +#include <string.h>
>> #include <xenevtchn.h>
>> #include <xengnttab.h>
>> #include <xenstore.h>
>> @@ -29,7 +30,6 @@
>>
>> #include <stdlib.h>
>> #include <errno.h>
>> -#include <string.h>
>> #include <poll.h>
>> #include <fcntl.h>
>> #include <unistd.h>
>
>
>
> [...]
>
>> menu "ARM errata workaround via the alternative framework"
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 49e1fb2..15efc13 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -52,6 +52,7 @@ obj-y += vm_event.o
>> obj-y += vtimer.o
>> obj-y += vpsci.o
>> obj-y += vuart.o
>> +obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o
>
>
> Please the alphabetical order. Just noticed vtimer is not correctly
> positioned. I will send a patch for that.
>
ok.
>
>> +#include <xen/errno.h>
>> +#include <xen/event.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/init.h>
>> +#include <xen/lib.h>
>> +#include <xen/mm.h>
>> +#include <xen/sched.h>
>> +#include <public/domctl.h>
>> +#include <public/io/console.h>
>> +#include <asm-arm/pl011-uart.h>
>> +#include <asm-arm/vgic-emul.h>
>> +#include <asm-arm/vpl011.h>
>> +
>> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
>> +{
>> + return (dabt.size != DABT_DOUBLE_WORD);
>
>
> Again, please add a comment explaining why we allow all the sizes but
> 64-bit.
>
>> +}
>> +
>> +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.
>
>> + 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. The memory barrier
after the 3 statements ensures that intf->in_cons is updated before
proceeding ahead.
>
>> + }
>> + else
>> + {
>> + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>> + }
>
>
> The {} are not necessary.
ok.
>
>> +
>> + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
>
>
> 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.
>
>> + {
>> + vpl011->uartfr |= RXFE;
>> + vpl011->uartris &= ~RXI;
>> + }
>> + vpl011->uartfr &= ~RXFF;
>> + VPL011_UNLOCK(d, flags);
>> +
>> + 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;
>
>
> See my remark above.
I will move index reading under lock.
>
>> +
>> + 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 ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>> + sizeof (intf->out) )
>> + {
>> + intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
>> + smp_wmb();
>> + out_prod += 1;
>> + intf->out_prod = out_prod;
>> + }
>> + else
>> + {
>> + gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>> + }
>
>
> The {} are not necessary.
ok.
>
>> +
>> + if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
>> + sizeof (intf->out) )
>
>
> Ditto here.
>
>
>> + {
>> + vpl011->uartfr |= TXFF;
>> + vpl011->uartris &= ~TXI;
>> + }
>> +
>> + vpl011->uartfr |= BUSY;
>> +
>> + vpl011->uartfr &= ~TXFE;
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + /*
>> + * Send an event to console backend to indicate that there is
>> + * data in the OUT ring buffer.
>> + */
>> + notify_via_xen_event_channel(d, vpl011->evtchn);
>> +}
>> +
>> +static int vpl011_mmio_read(struct vcpu *v,
>> + mmio_info_t *info,
>> + register_t *r,
>> + void *priv)
>> +{
>> + struct hsr_dabt dabt = info->dabt;
>> + uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>> + struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>> +
>> + switch ( vpl011_reg )
>> + {
>> + case DR:
>> + /*
>> + * Since pl011 registers are 32-bit registers, all registers
>> + * are handled similarly allowing 8-bit, 16-bit and 32-bit
>> + * accesses.
>> + */
>
>
> This comment should be on top of the declaration of
> vpl011_reg32_check_access.
ok.
>
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011_read_data(v->domain), info);
>> + return 1;
>> +
>> + case RSR:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + /* It always returns 0 as there are no physical errors. */
>> + *r = 0;
>> + return 1;
>> +
>> + case FR:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartfr, info);
>
>
> You need to ensure that uartfr is read only once because vreg_reg32_extract
> does not currently ensure that.
>
>> + return 1;
>> +
>> + case RIS:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartris, info);
>
>
> Ditto.
>
>> + return 1;
>> +
>> + case MIS:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartris &
>> + vpl011->uartimsc, info);
>
>
> Ditto.
>
>> + return 1;
>> +
>> + case IMSC:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartimsc, info);
>
>
> Ditto.
I will read these registers into a local variable and use it.
>
>
>> + return 1;
>> +
>> + case ICR:
>> + if ( !vpl011_reg32_check_access(dabt) ) 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)
>> +{
>> + struct hsr_dabt dabt = info->dabt;
>> + uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>> + struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>> + struct domain *d = v->domain;
>> + unsigned long flags;
>> +
>> + switch ( vpl011_reg )
>> + {
>> + case DR:
>> + {
>> + uint32_t data = 0;
>> +
>> + /*
>> + * Since pl011 registers are 32-bit registers, all registers
>> + * are handled similarly allowing 8-bit, 16-bit and 32-bit
>> + * accesses.
>> + */
>
>
> See above.
>
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + vreg_reg32_update(&data, r, info);
>> + data &= 0xFF;
>> + vpl011_write_data(v->domain, data);
>> + return 1;
>> + }
>> + case RSR: /* Nothing to clear. */
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + return 1;
>> +
>> + case FR:
>> + case RIS:
>> + case MIS:
>> + goto write_ignore;
>> +
>> + case IMSC:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + VPL011_LOCK(d, flags);
>> + vreg_reg32_update(&vpl011->uartimsc, r, info);
>> + VPL011_UNLOCK(d, flags);
>> + vpl011_update(v->domain);
>
>
> I think this should be call with under the lock.
>
>> + return 1;
>> +
>> + case ICR:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + VPL011_LOCK(d, flags);
>> + vreg_reg32_clearbits(&vpl011->uartris, r, info);
>> + VPL011_UNLOCK(d, flags);
>> + vpl011_update(d);
>
>
> Ditto.
>
>
>> + return 1;
>> +
>> + default:
>> + gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
>> + dabt.reg, vpl011_reg);
>> + return 0;
>> + }
>> +
>> +write_ignore:
>> + 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,
>> +};
>> +
>> +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;
>
>
> Same as above for the barrier.
>
>
>> + XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + in_ring_qsize = xencons_queued(in_prod,
>> + in_cons,
>> + sizeof(intf->in));
>> +
>> + out_ring_qsize = xencons_queued(out_prod,
>> + out_cons,
>> + sizeof(intf->out));
>> +
>> + /* Update the uart rx state if the buffer is not empty. */
>> + if ( in_ring_qsize != 0 )
>> + {
>> + vpl011->uartfr &= ~RXFE;
>> + if ( in_ring_qsize == sizeof(intf->in) )
>> + vpl011->uartfr |= RXFF;
>> + vpl011->uartris |= RXI;
>> + }
>> +
>> + /* Update the uart tx state if the buffer is not full. */
>> + if ( out_ring_qsize != sizeof(intf->out) )
>> + {
>> + vpl011->uartfr &= ~TXFF;
>> + vpl011->uartris |= TXI;
>> + if ( out_ring_qsize == 0 )
>> + {
>> + vpl011->uartfr &= ~BUSY;
>> + vpl011->uartfr |= TXFE;
>> + }
>> + }
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + vpl011_update(d);
>
>
> See my comment above for the calling vpl011_update
>
>> +}
>> +
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> + vpl011_data_avail(v->domain);
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>> +{
>> + int rc;
>> + struct vpl011 *vpl011 = &d->arch.vpl011;
>> +
>> + if ( vpl011->ring_buf )
>> + return 0;
>
>
> IHMO, you should return an error if the PL011 is already initialized. This
> should never happen.
ok.
>
>> +
>> + /* 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. I am removing the mmio handlers in the error
legs in domain_vpl011_init().
>
>> +
>> + 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.
>>
>> + 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.
>
>> + return;
>> +
>> + free_xen_event_channel(d, vpl011->evtchn);
>> + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
>> + xfree(d->arch.vmmio.handlers);
>> +}
>
>
> [...]
>
>
>> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
>> new file mode 100644
>> index 0000000..b3e332d
>> --- /dev/null
>> +++ b/xen/include/asm-arm/vpl011.h
>> @@ -0,0 +1,74 @@
>> +/*
>> + * 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_
>> +
>
>
> We tend to keep #ifndef and #define together. So please drop the newline
> here.
>
ok.
>> +#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?
>
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |