|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 03/12 v3] xen/arm: vpl011: Add pl011 uart emulation in Xen
Hi Julien,
Thanks for your comments.
>
>
>> +static bool vpl011_reg32_check_access(int size)
>
>
> Please pass hsr_dabt in parameter rather than the size directly. Which BTW
> should have really be unsigned int.
>
ok.
>> +{
>> + return (size == DABT_DOUBLE_WORD)? false : true;
>
>
> This could be simplified with (size != DABT_DOUBLE_WORD). Also please add a
> comment explaining why we allow all the sizes but 64-bit.
>
ok.
>> +}
>> +
>> +static void vpl011_update_spi(struct domain *d)
>
>
> Please rename this function to vpl011_update.
>
ok.
>> +{
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> + if ( vpl011->uartris & vpl011->uartimsc )
>
>
> Likely you want a todo to say we need to revisit that when the vGIC is
> handling properly level interrupt.
>
ok.
>> + vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>> +}
>> +
>> +static void vpl011_read_data(struct domain *d, uint8_t *data)
>
>
> If a pointer is passed to be filled, then I would expect an error value to
> be returned.
>
> I would usually pointer if the function has to fill a structure or it is not
> possible to use the return.
>
> In this case, the value is 8-bit and you don't have return value. So return
> the value rather than passing a pointer.
>
I will return the data.
>
>> + *data = intf->in[VPL011_RING_IDX_MASK(in_cons, intf->in)];
>> + smp_mb();
>> + intf->in_cons = in_cons + 1;
>> + }
>
>
> For debugging purpose, it would be good to have a message if the ring is
> empty.
ok.
>
>> +
>> + if ( VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + vpl011->uartfr |= RXFE;
>> + vpl011->uartris &= ~RXI;
>> + }
>> + vpl011->uartfr &= ~RXFF;
>> + VPL011_UNLOCK(d, flags);
>> +
>> + notify_via_xen_event_channel(d, vpl011->evtchn);
>
>
> It would be good to explain why you need to notification here.
>
On second thoughts, notification is not required here.
>
>> + intf->out[VPL011_RING_IDX_MASK(out_prod, intf->out)] = data;
>> + smp_wmb();
>> + intf->out_prod = out_prod + 1;
>> + }
>
>
> For debugging purpose, it would be useful to get a message if the ring is
> full.
>
ok.
>> +
>> + if ( VPL011_OUT_RING_FULL(intf) )
>> + {
>> + vpl011->uartfr |= TXFF;
>> + vpl011->uartris &= ~TXI;
>> + }
>> +
>> + vpl011->uartfr |= BUSY;
>
>
> I am not sure to understand why you set the bit BUSY here.
Since the OUT ring buffer is non-empty here, this bit is set to
indicate that there is data in the ring buffer (which is emulating a
UART transmit FIFO). As per pl011 TRM, this bit should be set if there
is data in the transmit fifo.
>
>> +
>> + vpl011->uartfr &= ~TXFE;
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + notify_via_xen_event_channel(d, vpl011->evtchn);
>
>
> Same as the previous notify_*.
>
ok. I will add a comment on why we are sending an event here.
>> +}
>> +
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
>> *r, void *priv)
>> +{
>> + uint8_t ch;
>
>
> This could be restricted to the case DR.
>
ok.
>> + struct hsr_dabt dabt = info->dabt;
>> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>
>
> Please use either unsigned int or uint32_t. Something would be really wrong
> if the offset is negative here.
ok.
>
>> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> + if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
>
>
> Please add a comment why the check is the same for all registers.
>
ok.
>> +
>> + switch ( vpl011_reg )
>> + {
>> + case DR:
>
>
> As mentioned above, you could do:
>
> {
> uint8_t ch;
> ....
> }
>
>> + vpl011_read_data(v->domain, &ch);
>> + *r = ch;
>
>
> Please use vreg_reg32_extract(...).
>
ok.
>> + break;
>
>
> I admit I would prefer the "return 1;" here rather than "break;". This makes
> easier to follow the emulation for a given register.
>
> I would even be in favor of duplicating the "if ( !vpl011... )" in each case
> for the same reason.
Do you mean that I should repeat the vpl011_reg32_check_access() and
return for each switch case?
>
>> +
>> + case RSR:
>> + /* It always returns 0 as there are no physical errors. */
>> + *r = 0;
>
>
> Note that I am ok to keep the 0 like that here.
>
>> + break;
>> +
>> + case FR:
>> + *r = vreg_reg32_extract(vpl011->uartfr, info);
>> + break;
>> +
>> + case RIS:
>> + *r = vreg_reg32_extract(vpl011->uartris, info);
>> + break;
>> +
>> + case MIS:
>> + *r = vreg_reg32_extract(vpl011->uartris
>> + & vpl011->uartimsc, info);
>
>
> The coding style in Xen is to split after the &. E.g
>
> vpl011->uartris &
> vpl011->uartimsc
>
ok.
> Also, none of the pl011 register emulation is using any locking. This is
> quite surprising to me. So can you explain why?
In vpl011_mmio_read(), since pl011 registers are only read, it should
be ok to let
them be read without any locking.
>
>
>> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info,
>> register_t r, void *priv)
>> +{
>> + uint8_t ch = ((struct uartdr_reg *)&r)->data;
>
>
> This should be restricted to the DR case below. But I am not sure to
> understand the purpose the structure here. It makes less readable and also
> you only use it for the write case.
There was a review comment earlier to make the typecasting more
readable because earlier I was doing r & 0xff to retrieve the data. So
I added a structure which tells the layout of the DR. I guess I can
use vreg_reg32_extract() to extract uint8_t value from register_t r.
>
>> + struct hsr_dabt dabt = info->dabt;
>> + int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>
>
> Same as above for vpl011_reg.
ok.
>
>> + struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> + if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
>
>
> Same as the previous vpl011_reg32_check_access... and all of my remarks in
> the read emulation stand for the write one.
>
>> +
>> + switch ( vpl011_reg )
>> + {
>> + case DR:
>> + vpl011_write_data(v->domain, ch);
>> + break;
>> +
>> + case RSR: /* Nothing to clear. */
>> + break;
>> +
>> + case FR:
>> + case RIS:
>> + case MIS:
>> + goto write_ignore;
>> +
>> + case IMSC:
>> + vreg_reg32_update(&vpl011->uartimsc, r, info);
>> + vpl011_update_spi(v->domain);
>
>
> I would have expected some locking here.
Yes here lock should be taken.
>
>> + break;
>> +
>> + case ICR:
>> + vreg_reg32_clearbits(&vpl011->uartris, r, info);
>> + vpl011_update_spi(v->domain);
>
>
> Same here.
Yes here lock should be taken.
>
>
>> + break;
>> +
>> + 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,
>> +};
>> +
>> +int vpl011_map_guest_page(struct domain *d, xen_pfn_t gfn)
>
>
> This function should either have the prototype defined in an header if used
> outside or static.
>
> Also, I have asked to use gfn_t and not xen_pfn_t. The former is a typesafe
> avoiding mix between MFN and GFN.
>
>> +{
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> + /* Map the guest PFN to Xen address space. */
>> + return prepare_ring_for_helper(d,
>> + gfn,
>> + &vpl011->ring_page,
>> + &vpl011->ring_buf);
>
>
> Looking at the usage it is only used in domain_vpl011_init. So I am not
> convinced of the of usefulness of this wrapper.
>
Yes. I will remove the wrapper function.
>> +}
>> +
>> +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;
>> + RING_IDX in_ring_depth, out_ring_depth;
>
>
> I am a bit confused with the term "depth". It does not seem to fit for a
> ring.
I will use in_ring_qsize and out_ring_qsize instead.
>
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + in_ring_depth = vpl011_queued(intf->in_prod, intf->in_cons,
>> sizeof(intf->in));
>
>
> You introduced a macro VPL011_RING_DEPTH for hiding vpl011_queued, but don't
> use it.
I have removed this macro as much of ring buffer manipultation is
handled through vpl011_queued()
and vpl011_mask().
>
>> + out_ring_depth = vpl011_queued(intf->out_prod, intf->out_cons,
>> sizeof(intf->out));
>
>
> Ditto.
>
>
>> +
>> + /* Update the uart rx state if the buffer is not empty. */
>> + if ( in_ring_depth != 0 )
>> + {
>> + vpl011->uartfr &= ~RXFE;
>> + if ( in_ring_depth == VPL011_RING_MAX_DEPTH(intf, in) )
>> + vpl011->uartfr |= RXFF;
>> + vpl011->uartris |= RXI;
>> + }
>> +
>> + /* Update the uart tx state if the buffer is not full. */
>> + if ( out_ring_depth != VPL011_RING_MAX_DEPTH(intf, out) )
>> + {
>> + vpl011->uartfr &= ~TXFF;
>> + vpl011->uartris |= TXI;
>> + if ( out_ring_depth == 0 )
>> + {
>> + vpl011->uartfr &= ~BUSY;
>> + vpl011->uartfr |= TXFE;
>> + }
>> + }
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + vpl011_update_spi(d);
>> +}
>> +
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> + vpl011_data_avail(v->domain);
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d,
>> + uint32_t console_domid,
>> + xen_pfn_t gfn,
>
>
> This should be gfn_t and not xen_pfn_t.
>
>> + evtchn_port_t *evtchn)
>> +{
>> + int rc;
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>
>
> What if domain_vpl011_init is called twice? After nothing seem to prevent
> the new DOMCTL to be called twice.
>
I will add a check in the init function to return without doing
anything if vpl011 is already initialized.
>> + rc = vpl011_map_guest_page(d, gfn);
>> + 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);
>
>
> 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.
>
I will free the mmio handlers in the vpl011 deinit function.
>> +
>> + spin_lock_init(&vpl011->lock);
>> +
>> + rc = alloc_unbound_xen_event_channel(d, 0, console_domid,
>> + vpl011_notification);
>> + if (rc < 0)
>
>
> Coding style:
>
> if ( ... )
>
ok.
>> + goto out2;
>> +
>> + vpl011->evtchn = *evtchn = rc;
>> +
>> + vpl011->initialized = true;
>
>
> I am not sure to understand the purpose of this variable. You only use it in
> domain_vpl011_deinit. But you can easily know if you need to free by looking
> at the state of ring_buf.
It is an extraneous variable. I will remove it.
>
>> +
>> + return 0;
>> +
>> +out2:
>> + xfree(d->arch.vmmio.handlers);
>> +out1:
>
>
> I would have expected vgic_free_virq to be called in case of error.
>
ok.
>> + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
>> +out:
>> + return rc;
>> +}
>> +
>> +void domain_vpl011_deinit(struct domain *d)
>> +{
>> + struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> + if ( vpl011->initialized )
>
>
> See my remark about vpl011->initialized above.
Yes, I will use the if ( vpl011->ring_buf ) check instead.
>
> In any case, I would prefer the condition to be inverted to avoid an extra
> layer of tab. I.e
>
> if ( !vpl011->initialized )
> return;
>
ok.
>
>> +#ifndef _VPL011_H_
>> +
>> +#define _VPL011_H_
>> +
>> +#include <public/io/ring.h>
>> +#include <asm-arm/vreg.h>
>> +
>> +DEFINE_XEN_FLEX_RING(vpl011);
>
>
> I am sure someone already said it in a previous version. The vpl011 is the
> console ring. So why are we defining our own internally?
>
> At least this should have been used by xenconsole, but this is not the
> case... So we should really avoid defining our own ring here and re-use
> public/io/console.h.
>
>
>> +
>> +struct uartdr_reg {
>> + uint8_t data;
>> + uint8_t error_status:4;
>> + uint8_t reserved1:4;
>> + uint16_t reserved2;
>> + uint32_t reserved3;
>> +};
>
>
> I don't see any benefits of this structure.
I will remove this structure and use the register value directly after
masking it.
>
>> +
>> +struct vpl011_s {
>
>
> Please remove the _s in the name.
ok.
>
>> + void *ring_buf;
>> + struct page_info *ring_page;
>> + uint32_t uartfr; /* Flag register */
>> + uint32_t uartcr; /* Control register */
>> + uint32_t uartimsc; /* Interrupt mask register*/
>> + uint32_t uarticr; /* Interrupt clear register */
>> + uint32_t uartris; /* Raw interrupt status register */
>> + uint32_t uartmis; /* Masked interrupt register */
>> + spinlock_t lock;
>> + evtchn_port_t evtchn;
>> + bool initialized; /* Flag which tells whether vpl011 is
>> initialized */
>> +};
>> +
>> +#ifdef CONFIG_VPL011_CONSOLE
>> +int domain_vpl011_init(struct domain *d,
>> + uint32_t console_domid,
>> + xen_pfn_t gfn,
>> + evtchn_port_t *evtchn);
>> +void domain_vpl011_deinit(struct domain *d);
>> +#else
>> +int domain_vpl011_init(struct domain *d,
>
>
> static inline here.
ok.
>
>> + uint32_t console_domid,
>> + xen_pfn_t gfn,
>> + evtchn_port_t *evtchn) { return -ENOSYS; }
>
>
> Please indent correctly { return -ENOSYS };
>
ok.
>> +
>> +static inline void domain_vpl011_deinit(struct domain *d) { }
>
>
> Note that I am ok with the { } on the same line here.
>
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index bd974fb..5f91207 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -322,6 +322,8 @@ struct xen_arch_domainconfig {
>> *
>> */
>> uint32_t clock_frequency;
>> +
>> + uint32_t console_domid;
>
>
> Why this is here?
I will move it out of here.
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |