Re: [PATCH v5] xen/console: introduce domain_console struct

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.07.2025 02:44, dmkhn@xxxxxxxxx wrote:
> On Wed, Jun 25, 2025 at 12:30:03AM +0000, dmkhn@xxxxxxxxx wrote:
>> From: Denis Mukhin <dmukhin@xxxxxxxx> 
>>
>> Introduce domain_console for grouping data structures used for integrating
>> domain's diagnostic console with Xen's console driver.
>>
>> Group all pbuf-related data structures under domain_console. Rename the moved
>> fields to plain .buf, .idx and .lock names, since all uses of the fields are
>> touched.
>>
>> Bump the domain console buffer size to the closest power of 2 (256) and
>> rename the symbol to DOMAIN_CONSOLE_BUF_SIZE.
>>
>> Finally, update the domain_console allocation and initialization code.
>>
>> No functional change.
>>
>> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Can I ask for review?

I think I did give enough comments on earlier versions? I'm not entirely
comfortable actually ack-ing this change. One aspect is, that - despite
earlier comments - ...

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -371,6 +371,25 @@ struct evtchn_port_ops;
>>  
>>  #define MAX_NR_IOREQ_SERVERS 8
>>  
>> +/*
>> + * Arbitrary value.
>> + * The buffer is part of the dynamically-allocated data structure, using an
>> + * even multiple of a cache line size may help to optimize the allocation
>> + * overhead.
>> + */
>> +#define DOMAIN_CONSOLE_BUF_SIZE 256

... the comment here still isn't quite accurate imo, and hence potentially
misleading. The overall size of ...

>> +/* Domain console settings. */
>> +struct domain_console {
>> +    /* Permission to take ownership of the physical console input. */
>> +    bool input_allowed;
>> +
>> +    /* hvm_print_line() and guest_console_write() logging. */
>> +    spinlock_t lock;
>> +    unsigned int idx;
>> +    char buf[DOMAIN_CONSOLE_BUF_SIZE];
>> +};

... this struct isn't necessarily guaranteeing minimal overhead just
becasue buf[] size is a multiple of the system cacheline size.

Jan