|
[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 Andre,
Thanks for your comments.
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index d46b98c..c1a0e7f 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -50,6 +50,11 @@ config HAS_ITS
>> prompt "GICv3 ITS MSI controller support" if EXPERT = "y"
>> depends on HAS_GICV3
>>
>> +config VPL011_CONSOLE
>> + bool "Emulated pl011 console support"
>> + default y
>> + ---help---
>> + Allows a guest to use pl011 UART as a console
>
> I admit that I am rather late with this comment, but I am not sure we
> should advertise PL011 emulation here.
> Technically what you emulate is a "SBSA Generic UART", which is indeed a
> subset of the PL011, but really only a subset. You confirm this already
> in patch 13/14, where you use the respective compatible name instead of
> the generic arm,pl011 one.
>
> So while I don't dare to ask for renaming every identifier, I wonder if
> we should at least keep the publicly visible part confined to "SBSA
> UART". You could mention the subset nature in the help message here, for
> instance.
> The same reasoning applies to other parts of this series which introduce
> user-visible strings (like in libxl).
>
>> endmenu
>>
I will rename the user visible part to "sbsa_uart".
>> +#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>
>
> I think Julien mentioned this already, but I think you should just use
> "#include <asm/.. .h>" here.
>
ok.
>> +
>> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
>> +{
>> + return (dabt.size != DABT_DOUBLE_WORD);
>> +}
>> +
>> +static void vpl011_update(struct domain *d)
>
> Can you please rename this function to indicate that it updates the
> *interrupt status*? That name as it is rather generic at the moment.
>
I will rename it to vpl011_update_interrupt_status.
>> +{
>> + 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 )
>> + vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>
> So my understanding is that this is using an edge triggered semantic at
> the moment. While this is not what an PL011 actually implements and not
> what the driver really expects, this is fine on itself for now.
> BUT I think we may want to avoid injecting spurious interrupts now:
> If for instance the receive interrupt condition is set and we clear the
> transmit interrupt bit, then call this function, it would inject a new
> interrupt, although in a edge-triggered world we really wouldn't need to
> do anything.
> So I believe we would need to have some kind of shadowed interrupt
> state, which stores the interrupt condition the guest already knows
> about. As soon as we *add* a bit to this state, we inject the SPI.
> This would act as a kind of temporary bridge between the level triggered
> SBSA/PL011 UART and the edge-only VGIC implementation for now.
> Later when the VGIC properly handles level triggered interrupts, this
> implementation can be adjusted. But this change should then be confined
> to this very function.
>
ok. I will update the logic accordingly.
>> +}
>> +
>> +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;
>> +
>> + 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;
>
> In a level triggered world you would need to possibly change the status
> of the interrupt line here, so a call to (a renamed and fixed)
> vpl011_update() function would be due here. To make the transition later
> as easy as possible, I'd recommend to implement this shadow interrupt
> state as described above and then call the interrupt update function
> here already.
ok.
>
>> + }
>> + 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;
>> +
>> + 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");
>> + }
>> +
>> + if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
>> + sizeof (intf->out) )
>> + {
>> + vpl011->uartfr |= TXFF;
>> + vpl011->uartris &= ~TXI;
>
> Same as above. Basically whenever you change one of the bits that may
> affect the status of the interrupt line, I'd call the update function.
ok.
>
>> + }
>> +
>> + 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.
>> + */
>> + 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);
>> + return 1;
>> +
>> + case RIS:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartris, info);
>> + return 1;
>> +
>> + case MIS:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartris &
>> + vpl011->uartimsc, info);
>
> That smells like you need a lock here, since another access could change
> either RIS and/or IMSC concurrently.
> But as Julien already mentioned, these accesses are prone to races
> anyway, since vreg_reg32_extract() is not atomic.
> So is it worth to take the lock here just before the whole switch statement?
>
Yes, I have taken the lock while reading.
>> + return 1;
>> +
>> + case IMSC:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + *r = vreg_reg32_extract(vpl011->uartimsc, info);
>> + 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.
>> + */
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + vreg_reg32_update(&data, r, info);
>> + data &= 0xFF;
>
> This is not needed as vpl011_write_data()'s second argument is uint8_t,
> so the compiler does this masking anyway.
> And even if it wouldn't, you could move this statement into the call.
>
Yes. I will remove the masking.
>> + 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);
>> + return 1;
>> +
>> + case ICR:
>> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> + VPL011_LOCK(d, flags);
>> + vreg_reg32_clearbits(&vpl011->uartris, r, info);
>
> Please call the interrupt status update function here.
>
ok.
>> + VPL011_UNLOCK(d, flags);
>> + vpl011_update(d);
>> + 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;
>> + 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;
>
> ... and here ...
>
>> + }
>> +
>> + /* Update the uart tx state if the buffer is not full. */
>> + if ( out_ring_qsize != sizeof(intf->out) )
>> + {
>> + vpl011->uartfr &= ~TXFF;
>> + vpl011->uartris |= TXI;
>
> ... and here.
>
ok
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |