[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 05/05/17 12:18, Bhupinder Thakur wrote:
Hi Julien,

Hi Bhupinder,

+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.

The part "The SBSA spec specifies that the base address of the access..." should have been specified in the commit message because reading at the PL011 spec, I don't see this limit.

The reason of using vreg_reg* is to have all MMIOs emulation using the same helpers and avoid everyone to re-invent the wheel because you can "optimize" for your case.

Also, it is also more obvious to read vreg_reg32_update(...) than "& vpl011_reg_mask[dabt.size]". This would avoid quite a lot rework if we ever decide to modify the reg emulation.

+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

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

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

The code should only call a function vpl011_update that will clear or set the line. I don't see why it would need to specifically call clear/set.

+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.

Then it should have been written in the commit message. I made this comment on the previous version and didn't see any answer from you. So I considered you forgot to address it.

A general rule is to answer on the review e-mail or specify after "---" why you didn't address a comment so we know why it has not been done. Reviewers may guess wrong why you didn't do it :).


Missing Emacs magic.
Can you please elaborate this comment?

All files in Xen contains the below lines to help e-macs to load the correct format:

 * Local variables:
 * mode: C
 * c-file-style: "BSD"
 * c-basic-offset: 4
 * indent-tabs-mode: nil
 * End:

This should be added on any new files using Xen coding style.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.