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

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

On 23/03/17 09:14, Bhupinder Thakur wrote:
> .Hi Julien,

Hi Bhupinder,

> On 5 March 2017 at 17:42, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Bhupinder,
>>
>> On 21/02/17 11:25, Bhupinder Thakur wrote:
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>
>> Lastly, what if vpl011_write_data is returning an error?
>>
> Normally vpl011_write_data() should not fail since the guest should
> stop writing more data once the ring buffer goes full and TXFF bit is
> set in UARTFR.
> So there should always be space in the ring buffer for the next data
> when a mmio write happens.
>
> If the guest still writes when ring buffer is already full then data
> would be lost.

If the return code is meaningless, then the function should be void. 
Also, the behavior should be documented.

[...]

>>> +{
>>> +    int rc=0;
>>> +
>>> +    /*
>>> +     * register xen event channel
>>> +     */
>>> +    rc = alloc_unbound_xen_event_channel(d, 0,
>>> current->domain->domain_id,
>>> +
>>> vpl011_notification);
>>
>>
>> This line looks wrong to me. The console backend may not live in the same
>> domain as the toolstack.
>
> How should I figure out the correct domain where xenconsoled is
> running?

I think you would to pass the console domain id from the toolstack (see 
console->backend_domid).

>>
>>> +    if (rc < 0)
>>> +    {
>>> +        printk ("Failed to allocate vpl011 event channel\n");
>>> +        return rc;
>>> +    }
>>> +    d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc;
>>> +
>>> +    /*
>>> +     * allocate an SPI VIRQ for the guest
>>> +     */
>>> +    d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] =
>>> vgic_allocate_spi(d);
>>
>>
>> It makes little sense to hardcode the MMIO region and not the SPI. Also, I
>> would rather avoid to introduce new HVM_PARAM when not necessary. So
>> hardcoding the value looks more sensible to me.
>>
> So for reserving a SPI for vpl011, should I reserve one bit in the
> vgic.allocated_irqs bitmap in domain_vgic_init(), say 988, for vpl011?
> I think if I am going to reserve 1 SPI for vpl011 then I need not bump
> up nr_spis by 1.

Regardless the solution solution, nr_spis still need to be incremented. 
This field is used to know the number of SPIs exposed to the guest.

Cheers,

-- 
Julien Grall

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