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

From: Bhupinder Thakur <bhupinder.thakur@linaro.org>
To: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Andre Przywara <andre.przywara@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen
Date: Sat, 6 May 2017 10:50:43 +0530	[thread overview]
Message-ID: <CACtJ1JTm0dgm09hAZJdvAH5jnFmCWWjzMABhfCE3Qimdd_pYTA@mail.gmail.com> (raw)
In-Reply-To: <36f675ff-079f-50a5-84da-60e0ced56084@arm.com>

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.
>
>
> 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.
>
ok. I will redefine the vgic_reg* functions to generic vreg_reg* and
move the definitions to vreg.h, so that those can be used by vpl011
also.
+
>
>>>
>>>> +
>>>> +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.
>
ok. I will rename vgic_inject_vpl011_spi() to vpl011_update_irq().

>>
>>>
>>>> +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 :).
ok. I will update the commit message accordingly.

>
> [...]
>
>>> 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.
>
Thanks. I will add this to the new files.

Regards,
Bhupinder

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