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

[Julien Grall <julien.grall@arm.com>]

On 03/21/2017 01:27 PM, Bhupinder Thakur wrote:
> Hi Julien,

Hi Bhupinder,

> On 26 February 2017 at 22:37, Julien Grall <julien.grall@arm.com> wrote:
>>> +            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.

What you describe is how a well-behave guest will interact with the 
pl011 emulation. It does not describe what would happen if a misbehaved 
guest will read continuously the register.

I cannot find any things in the spec about what should be the state of 
the register if no data is available. So I guess we just need to ensure 
that we don't leak in information from the stack. This seems to be 
addressed by *data = 0 in "vpl011_read_data".

Please document it to avoid removing it by mistake in the future.

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

Fair point. I was more worry about ordering between read/write between 
multiple vCPU. But I guess we don't care if the read does not return an 
update to date value.

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

I don't see any behavior requirement in the spec. So I would send an 
abort to the guest (e.g return 0 in the function).

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

Sorry I still don't get it. Xenconsole will likely have to hold 
characters that are not written in the PV console. I would have expected 
that this would be used to set the OE bit. Did I miss anything?

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

No. I want to replace 0/1 by false/true but I never had the time to do 
the work.

Cheers,

-- 
Julien Grall

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