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

[Julien Grall <julien.grall@xxxxxxx>]

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

Cheers,

--
Julien Grall

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