Re: [PATCH v3 4/9] x86/PVH: provide VGA console info to Dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2021 17:01, Roger Pau Monné wrote:
> On Tue, Sep 21, 2021 at 09:18:05AM +0200, Jan Beulich wrote:
>> --- a/xen/include/public/arch-x86/hvm/start_info.h
>> +++ b/xen/include/public/arch-x86/hvm/start_info.h
>> @@ -33,7 +33,7 @@
>>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>   *  4 +----------------+
>> - *    | version        | Version of this structure. Current version is 1. 
>> New
>> + *    | version        | Version of this structure. Current version is 2. 
>> New
>>   *    |                | versions are guaranteed to be backwards-compatible.
>>   *  8 +----------------+
>>   *    | flags          | SIF_xxx flags.
>> @@ -55,7 +55,15 @@
>>   *    |                | if there is no memory map being provided. Only
>>   *    |                | present in version 1 and newer of the structure.
>>   * 52 +----------------+
>> - *    | reserved       | Version 1 and newer only.
>> + *    | vga_info.offset| Offset of struct dom0_vga_console_info from base of
> 
> I'm not sure we are supposed to reference external structures like
> that. We took a lot of care to not depend on other headers, and to
> make this as agnostic as possible (IIRC KVM is also capable of using
> the PVH entry point natively, and hence depends on this header).

But KVM wouldn't be using a Dom0-only part of the interface, would
it? (I'm aware of the possible re-using of the entry point.)

> IF we want to add support for dom0_vga_console_info I think we need to
> at least provide a binary layout for it, like all the other pieces
> that are part of the HVM start info.

Which then means we can't sensibly re-use the existing structure,
as that doesn't have as strict rules as the hvm_start_info one.
Which in turn means Linux can't re-use the code converting
dom0_vga_console_info, resulting in two places needing updating
whenever information gets add to (then) both structures (what
information they carry will, after all, want to remain in sync).

>> + *    |                | struct hvm_start_info. Optional and only present in
>> + *    |                | version 2 and newer of the structure when
>> + *    |                | SIF_INITDOMAIN is set; zero if absent.
> 
> We have usually used an absolute physical address to reference other
> data, and I think we should likely keep in that way for coherency.

Hmm. (See below.)

>> + * 54 +----------------+
>> + *    | vga_info.size  | Size of present parts of struct 
>> dom0_vga_console_info.
>> + *    |                | Optional and only present in version 2 and newer of
>> + *    |                | the structure when SIF_INITDOMAIN is set; zero if
>> + *    |                | absent.
>>   * 56 +----------------+
>>   *
>>   * The layout of each entry in the module structure is the following:
>> @@ -139,7 +147,15 @@ struct hvm_start_info {
>>      uint32_t memmap_entries;    /* Number of entries in the memmap table.   
>>  */
>>                                  /* Value will be zero if there is no memory 
>>  */
>>                                  /* map being provided.                      
>>  */
>> -    uint32_t reserved;          /* Must be zero.                            
>>  */
> 
> This 'Must be zero' comment troubles me a bit, I'm not convinced we
> can just place data here (ie: it would no longer be 0). It's even
> worse because the must be zero comment is only present in the C
> representation of the struct, the layout above just denotes the field
> is reserved (which would imply it's fine to use for other means in
> v2).

I thought the textual description was meant to be the ABI spec. The C
comment should therefore be viewed as if missing "in version 1" or
"presently".

Taking into account also Andrew's reply, I have to admit that I'm
inclined to request that one of the two of you fix this obvious
shortcoming in both Xen and Linux. I'm not really willing to be the one
to introduce a 2nd layout for the same set of data just for the purpose
of "playing nice" in an area where that, affecting Dom0 only, doesn't
seem to matter all this much. My goal was rather to keep the impact on
hvm_start_info as low as possible (and in particular avoid changing its
size, as strictly speaking Linux'es consumer implementation is buggy:
It would always copy as much data as it knows _may_ be present, not as
little data as may have been _actually_ provided; whoever implemented
this did only consider one half of the compatibility requirements,
quite likely simply because in the design this aspect was also missed,
or else the structure would have had a length field right from its
introduction).

IOW I'm afraid I may not be seeing the "big picture" here ...

Jan