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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,

On 26 February 2017 at 22:37, Julien Grall <julien.grall@xxxxxxx> wrote:
>> +
>> +#include <xen/config.h>
>
>
> xen/config.h is included by default. Please drop it.
Removed.

>
>> +#include <xen/init.h>
>> +#include <xen/lib.h>
>> +#include <xen/errno.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/sched.h>
>> +#include <xen/monitor.h>
>
>
> Why do you need to include monitor.h?
>
>> +#include <xen/event.h>
>> +#include <xen/vmap.h>
>> +
>> +#include <xsm/xsm.h>
>
>
> Ditto.
>
>> +
>> +#include <public/xen.h>
>> +#include <public/hvm/params.h>
>> +#include <public/hvm/hvm_op.h>
>> +
>> +#include <asm/hypercall.h>
>
>
> Ditto.
>
I have removed the unncessary includes. I probably added them when I
initially created the file.

>> +#include "vpl011.h"
>> +
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
>> *r, void *priv)
>> +{
>> +    unsigned char ch;
>> +
>> +    switch (info->gpa - GUEST_PL011_BASE)
>
>
> Coding style:
>
> switch ( ... )
>
>> +    {
>> +        case VPL011_UARTCR_OFFSET:
>
>
> Coding style: the case should be aligned to {. E.g
>
> {
> case ...
>
Switch/case style corrected.

> Also, I would prefer if you don't include _OFFSET in all the name.
>
Removed the _OFFSET.

> Furthermore, can you please order the case by offset in the MMIO region.
> This would help to find if a register has been emulated or not.
>
Ordered the offsets in increasing order.

> Lastly, the user may have requested to read 8-bit, 16-bit, 32-bit. But you
> always return a 32-bit. Is a guest allowed to access using any size and in
> the middle of a register? Give a look on what is done for vgic-v{2,3}.c to
> check the size and read register. You may want to pull some code out to
> re-use here.
>
>> +            *r = v->domain->arch.vpl011.control;
>
>
> Similarly, I would prefer if the name of the field match the register name.
>
Renamed the vpl011 fields match the corresponding register names.

>> +            break;
>> +        case VPL011_UARTDR_OFFSET:
>> +            vpl011_read_data(v->domain, &ch);
>
>
> Should not you check the return value of vpl011_read_data? Also, what if
> there is no data?
This condition should not happen because the RX FIFO empty bit would
be set in the UARTFR register when the last data is read from the ring
buffer and the the guest is not supposed to issue next read until the
RX FIFO empty bit is cleared indicating there is more data now.

>
>> +            *r = ch;
>> +            break;
>> +        case VPL011_UARTFR_OFFSET:
>> +            *r = v->domain->arch.vpl011.flag;
>
>
> I am fairly surprised that none of this code is actually protected by lock.
> For instance the update of flag is not atomic. So is it safe?
For reading, I thought no locking was required, as I believe a 32-bit
value read/write on ARM should be atomic. So if the value is modified
in some other context while it is being read in another context, the
reader should see either the old value or the new value.
For register writes, yes I need to take a lock where I am updating
certain bits in the register. I will add the locking there.

>
>> +            break;
>> +        case VPL011_UARTIMSC_OFFSET:
>> +            *r = v->domain->arch.vpl011.intr_mask;
>> +            break;
>> +        case VPL011_UARTICR_OFFSET:
>> +            *r = 0;
>
>
> Looking at the spec, this register is write-only. So why do you implement
> RAZ?
In such cases, where the guest tries to write to RO register or tries
to read a WO register, should I send a abort to the guest?

>
>> +            break;
>> +        case VPL011_UARTRIS_OFFSET:
>> +            *r = v->domain->arch.vpl011.raw_intr_status;
>> +            break;
>> +        case VPL011_UARTMIS_OFFSET:
>> +            *r = v->domain->arch.vpl011.raw_intr_status &
>> +                                v->domain->arch.vpl011.intr_mask;
>> +            break;
>> +        case VPL011_UARTDMACR_OFFSET:
>> +            *r = 0; /* uart DMA is not supported. Here it always returns
>> 0 */
>
>
> My understanding of the spec is DMA is not optional. So what would happen if
> the guest tries to enable it?
>
>> +            break;
>> +        case VPL011_UARTRSR_OFFSET:
>> +            *r = 0; /* it always returns 0 as there are no physical
>> errors */
>
>
> This register contains contains the bit OE that tells whether the FIFO is
> full or not. The FIFO here is the PV ring, so maybe we should set this bit
> if the ring is full.
The OE condition will not happen in this case since xenconsole will
not write more data to the guest if the ring buffer is full. There is
a separate UARTFR status bit which indicates whether the ring buffer
is full.

>
>> +            break;
>> +        default:
>> +            printk ("vpl011_mmio_read: invalid switch case %d\n",
>> (int)(info->gpa - GUEST_PL011_BASE));
>
>
> Coding style: printk(...).
>
> Also, printk is not ratelimited by default. Please use gprintk(...) which
> will be ratelimited and print the domain information. This is useful when
> you have multiple guest.
>
Replaced printk with gprintk.

>> +            break;
>> +    }
>> +
>> +    return VPL011_EMUL_OK;
>
>
> Please use plain value as the return is not pl011 specific. Also, I am a bit
> surprised that you return "ok" even when a register as not been emulated.
> IHMO, a data abort should be sent to the guest.

Corrected this. Now it returns an error incase the register is not
emulated . For the return values, I see that typically values 1/0 are
returned like in vgic-v2/3.c. Are there some common macros which I can
use?

>
> Furthermore, looking at the overall function, I think it should be better if
> you re-use the template used by the other emulation (see vgic-v2.c) for
> instance. They have labels that helps to know what's going on.
>
> I will stop here in the review for today and will comment the rest tomorrow.
>
> Cheers,

Regards,
Bhupinder

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