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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 23, 2021 at 11:46:48AM +0200, Jan Beulich wrote:
> 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.)

I think naming it as dom0-only is part of the problem. In principle I
don't see why you couldn't use such structure to report VGA console
information when inside of a VM that has such device.

> > 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).

Indeed. I don't think there's a way for us to cut corners here. As
said above, I don't think we should explicitly limit the usage of this
information to Xen dom0, and hence should be a first class addition to
the start info contents.

> >> + *    |                | 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).

The isn't a strict need to have several different layouts on the
consumer, as you could easily use offsetof to copy up to the boundary
you know it's populated given a version number. But yes, a size field
would be useful to prevent the consumer from having to know the
version boundaries.

I can try to take a stab at implementing this at a later point, I
certainly don't want to force you into implementing something you
believe it's not the correct solution.

Thanks, Roger.