Re: [Xen-devel] [PATCH 08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Wei Liu,

Thanks for your comments.

On 12 April 2017 at 22:03, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> On Mon, Apr 03, 2017 at 03:14:31PM +0530, Bhupinder Thakur wrote:
>> Xenconsole supports only PV console currently. To get access to emulated 
>> pl011
>> uart another backend console is required.
>>
>> This patch modifies different data structures and APIs used
>> in xenconsole to support two console types: PV and VCON (virtual console).
>>
>> Change summary:
>>
>> 1. Modify the domain structure to support two console types: PV and a
>>    virtual console (VCON).
>>
>> 2. Modify different APIs such as buffer_append() to take a new parameter
>>    console_type as input and operate on the data structures indexed by
>>    the console_type.
>>
>> 3. Modfications in domain_create_ring():
>>     - Bind to the vpl011 event channel obtained from the xen store as a
>>       new
>>       parameter
>>     - Map the PFN to its address space to be used as IN/OUT ring
>>       buffers.
>>       It obtains the PFN from the xen store as a new parameter
>>
>> 4. Modifications in handle_ring_read() to handle both PV and VCON
>>    events.
>>
>> 5. Add a new log_file for VCON console logs.
>>
>
> It seems that this patch and previous one should be merged into one.
>
Yes I have merged this patch with the earlier one to maintain the bisectability.

> There are a lot of coding style issues, please fix all of them.
>
I will fix the coding style issues.

> The code looks rather repetitive unfortunately. I believe a lot of the
> repetitiveness can be avoided by using loops and pointers.
>
Stefano suggested to define an array of a console structure instead of
using different arrays for console related fields in the domain
structure.
I think that way the changes to the code will be much cleaner. I will
incorporate those changes.

>>  static void domain_close_tty(struct domain *dom)
>>  {
>> -     if (dom->master_fd != -1) {
>> -             close(dom->master_fd);
>> -             dom->master_fd = -1;
>> +     if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
>> +             close(dom->master_fd[CONSOLE_TYPE_PV]);
>> +             dom->master_fd[CONSOLE_TYPE_PV] = -1;
>> +     }
>> +
>> +     if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
>> +             close(dom->slave_fd[CONSOLE_TYPE_PV]);
>> +             dom->slave_fd[CONSOLE_TYPE_PV] = -1;
>> +     }
>> +
>> +     if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
>> +             close(dom->master_fd[CONSOLE_TYPE_VCON]);
>> +             dom->master_fd[CONSOLE_TYPE_VCON] = -1;
>>       }
>>
>> -     if (dom->slave_fd != -1) {
>> -             close(dom->slave_fd);
>> -             dom->slave_fd = -1;
>> +     if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
>> +             close(dom->slave_fd[CONSOLE_TYPE_VCON]);
>> +             dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
>>       }
>
> You can use two loops to avoid repetitive snippets. But I'm fine with
> the code as-is, too.
I have added a new function console_close_tty() which is called for
both pv and vuart consoles. The console
is abstracted as a separate structure.

static void console_close_tty(struct console *con)
{
    if (con->master_fd != -1) {
        close(con->master_fd);
        con->master_fd = -1;
    }

    if (con->slave_fd != -1) {
        close(con->slave_fd);
        con->slave_fd = -1;
    }
}

Other functions such as buffer_append(), handle_tty_read() will also
operate on the console structure.

>> @@ -1166,6 +1346,16 @@ void handle_io(void)
>>
>>               for (d = dom_head; d; d = n) {
>>                       n = d->next;
>> +
>> +                     /*
>> +                      * Check if the data pending flag is set for any of 
>> the consoles.
>> +                      * If yes then service those first.
>> +                      */
>> +                     if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
>> +                             buffer_append(d, CONSOLE_TYPE_PV);
>> +                     else if ( d->console_data_pending & 
>> (1<<CONSOLE_TYPE_VCON) )
>> +                             buffer_append(d, CONSOLE_TYPE_VCON);
>> +
>
> Why? You seem to have skipped the ratelimit check here.

I have thought about this change and I think this check is not
required. The rate limit check will ensure that more data is not
appended to the buffer as it will keep the events masked and
handle_ring_read() will not be called. I will remove this check.

Regards,
Bhupinder

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