Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,

>> +
>> +unsigned int vpl011_reg_mask[] = {0xff, 0xffff, 0xffffffff};
>
>
> This should be static. But I don't get what you need that. In the first
> version, I suggested to re-purpose vgic_reg*_{extract,update} so we can use
> it here. It would probably need to be renamed to vreg_reg*.
>
> I don't see any reason to not do that and re-inventing the wheel.

I understand that the vreg_reg* functions are handy in scenarios where
user may want to access the data at some offset from the register base
address. The SBSA spec specifies that the base address of the access
must be same as the base address of the register. So in this case the
offset would always be 0. That is the reason I used a simple mask to
return 8-bit, 16-bit and 32-bit values.
>
>> +
>> +static void vgic_inject_vpl011_spi(struct domain *d)
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    if ( (vpl011->uartris & vpl011->uartimsc) )
>> +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>> +}
>
>
> PL011 is using level interrupt which means that you should not inject
> interrupt but instead set or clear the pending interrupt.
>
> However, the problem is because the vGIC is incapable to handle properly
> level interrupt. This is going to be a major problem as the interrupt should
> stay pending until the pl011 emulation says there are no more interrupts to
> handle.
>
> For instance, you may miss character if the guest driver had not enough
> space to read character new one because the interrupt will not get
> re-injected.
>
> I am not asking to modify the vGIC in order to handle level properly (Andre
> in CC is looking at that). But we need to get the code in correct shape in
> order to handle properly pl011 interrupt.
>
> By that I mean, at least the naming of the function (I haven't read the rest
> to know what is missing). I.e I would rename to vpl011_update(...).
Should I define two functions vgic_vpl011_spi_set() and
vgic_vpl011_spi_clear()? For now, I can keep vgic_vpl011_spi_clear()
empty and rename
vgic_inject_vpl011_spi() to vgic_vpl011_spi_set(). I will call
vgic_vpl011_spi_clear() at all places where IN ring buffer has become
empty.

>
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
>> *r, void *priv)
>> +{
>> +    uint8_t ch;
>> +    struct hsr_dabt dabt = info->dabt;
>> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> +    switch ( vpl011_reg )
>> +    {
>> +    case DR:
>
>
> As said on the first version, all those registers have specific size. Please
> have a look at how we handle register emulation in the vgic with VREG*.
Since SBSA specs mandate that pl011 register accesses must always be
accessed using the register base address, I am using the register base
address here instead of an address range.
>
>> +        if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> +        vpl011_read_data(v->domain, &ch);
>> +        *r = ch;
>> +        break;
>> +
>> +    case RSR:
>> +        if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> +
>> +        /* It always returns 0 as there are no physical errors. */
>> +        *r = 0;
>> +        break;
>> +
>> +    case FR:
>> +        if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> +        *r = (vpl011->uartfr & vpl011_reg_mask[dabt.size]);
>
>
> Pointless ().
>
I will remove () at all relevant places.

>> +
>> +write_ignore:
>> +    if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>
>
> Why? Looking at the spec, write-ignore register does not have specific
> access size.
I will check.

>
>> +    return 1;
>> +
>> +word_write_ignore:
>> +    if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> +    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,
>> +};
>> +
>> +int vpl011_map_guest_page(struct domain *d, unsigned long pfn)
>
>
> Please don't use the term pfn as we don't know whether it refers to the
> guest frame number (GFN) or machine frame number (MFN).
>
> So in this case, I think it is gfn. Also s/unsigned long/gfn_t/
>
ok.
>
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    /* Map the guest PFN to Xen address space. */
>> +    return prepare_ring_for_helper(d,
>> +                                   pfn,
>> +                                   &vpl011->ring_page,
>> +                                   &vpl011->ring_buf);
>> +}
>> +
>> +static void vpl011_data_avail(struct domain *d)
>> +{
>> +    unsigned long flags;
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +    struct xencons_interface *intf = vpl011->ring_buf;
>> +    uint32_t in_ring_depth, out_ring_depth;
>> +
>> +    VPL011_LOCK(d, flags);
>> +
>> +    in_ring_depth = intf->in_prod - intf->in_cons;
>> +    out_ring_depth = intf->out_prod - intf->out_cons;
>> +
>> +    /* Update the uart rx state if the buffer is not empty. */
>> +    if ( in_ring_depth != 0 )
>> +    {
>> +        vpl011->uartfr &= ~(RXFE);
>
>
> Pointless ()
>
I will remove () from all relevant places.

>
>> +        }
>> +    }
>> +
>> +    VPL011_UNLOCK(d, flags);
>> +
>> +    vgic_inject_vpl011_spi(d);
>> +}
>> +
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> +    vpl011_data_avail(v->domain);
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d, struct xen_arch_domainconfig
>> *config)
>> +{
>> +    int rc;
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    rc = alloc_unbound_xen_event_channel(d, 0, config->console_domid,
>> +                                         vpl011_notification);
>> +    if (rc < 0)
>> +    {
>> +        return rc;
>> +    }
>> +
>> +    vpl011->evtchn = rc;
>> +    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>> +    if ( !rc )
>> +    {
>> +        free_xen_event_channel(d, vpl011->evtchn);
>> +        vpl011->evtchn = -1;
>> +        return rc;
>> +    }
>> +    register_mmio_handler(d, &vpl011_mmio_handler, GUEST_PL011_BASE,
>> GUEST_PL011_SIZE, NULL);
>> +    spin_lock_init(&vpl011->lock);
>> +
>> +    vpl011->initialized = true;
>> +
>> +    return 0;
>> +}
>> +
>> +void domain_vpl011_deinit(struct domain *d)
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    if ( vpl011->initialized )
>> +    {
>> +        free_xen_event_channel(d, vpl011->evtchn);
>> +        destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
>> +    }
>> +    vpl011->initialized = false;
>> +}
>
>
> Missing Emacs magic.
Can you please elaborate this comment?

>>
>>  struct hvm_domain
>>  {
>> @@ -133,6 +134,8 @@ struct arch_domain
>>      struct {
>>          uint8_t privileged_call_enabled : 1;
>>      } monitor;
>> +
>> +    struct vpl011_s vpl011;
>
>
> This should be guard by #ifdef CONFIG_VPL011_CONSOLE.
>
ok.

>> +#ifndef _VPL011_H_
>> +
>> +#define _VPL011_H_
>> +
>> +/* helper macros */
>> +#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir
>> ## _cons))
>> +
>> +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1)
>> +
>> +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0)
>> +
>> +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0)
>> +
>> +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) ==
>> VPL011_RING_MAX_DEPTH(intf, in))
>> +
>> +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) ==
>> VPL011_RING_MAX_DEPTH(intf,out))
>> +
>> +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock,
>> flags)
>> +#define VPL011_UNLOCK(d,flags)
>> spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
>> +
>> +#define VALID_BW_SIZE(size) ( size == DABT_BYTE || size == DABT_HALF_WORD
>> || size == DABT_WORD )
>> +#define VALID_W_SIZE(size)  ( size == DABT_HALF_WORD || size == DABT_WORD
>> )
>
>
> The header should only contain what needs to be used by other code. In this
> case we don't want anyone to use VALID_*_SIZE.
>
> Also, I find the name quite confusing, it is not clear when to use which
> one. Looking at the SBSA (6.2 in ARM-DEN-0029 v3.0): "if an access size not
> listed in the table is used, the results are IMPLEMENTATION DEFINED".
>
> In order to get the code simple I would handle all 32bits register in the
> same way (e.g 8-bits, 16-bits and 32-bits access always allowed).
>
> So I would introduce a static inline helper vpl011_reg32_check_access that
> check the return we don't use 64-bit access.
>
ok.
>

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel