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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,


>>>> +            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.
>
Ok. I will add a comment.

>>
>>>
>>>> +            *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).
>
Ok.
>>>
>>>> +            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?
I think OE bit is more applicable to physical UART, where data may
arrive while the hardware RX FIFO is already full. In that case, it
has to drop the incoming data but it also sets the OE bit to indicate
that overflow condition happened. In our case, since the ring buffer
acts as a flow control mechanism (i.e. if ring buffer is full then
xenconsole stops writing more data and hold the data) this condition
may not happen.

The pl011 emulation code does not know if xenconsole is holding the
data when the ring buffer is full. One thing we could do is to treat
the ring buffer full condition as an indication that overflow is
likely to happen and set this bit. Typically, the data rate from
xenconsole to the guest would be very low for this condition to ever
happen.
>
>>>
>>>> +            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.
>
I will use true/false.

Regards,
Bhupinder

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