Like PV Dom0 in order to use the console if in a mode other than text
80x25 the kernel needs to be provided information about this mode. Bump
HVM start info's "current" version to 2 and use a previously reserved
32-bit field to provide struct dom0_vga_console_info's position and
size.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -19,6 +19,7 @@
*/
#include <xen/acpi.h>
+#include <xen/console.h>
#include <xen/init.h>
#include <xen/libelf.h>
#include <xen/multiboot.h>
@@ -549,6 +550,11 @@ static int __init pvh_load_kernel(struct
paddr_t last_addr;
struct hvm_start_info start_info = { 0 };
struct hvm_modlist_entry mod = { 0 };
+#ifdef CONFIG_VIDEO
+ struct dom0_vga_console_info vga_info = { 0 };
+#else
+ struct {} __maybe_unused vga_info;
+#endif
struct vcpu *v = d->vcpu[0];
int rc;
@@ -598,7 +604,7 @@ static int __init pvh_load_kernel(struct
* split into smaller allocations, done as a single region in order to
* simplify it.
*/
- last_addr = find_memory(d, &elf, sizeof(start_info) +
+ last_addr = find_memory(d, &elf, sizeof(start_info) + sizeof(vga_info) +
(initrd ? ROUNDUP(initrd->mod_end, PAGE_SIZE) +
sizeof(mod)
: 0) +
@@ -672,6 +678,22 @@ static int __init pvh_load_kernel(struct
last_addr += sizeof(mod);
}
+#ifdef CONFIG_VIDEO
+ if ( fill_console_start_info(&vga_info) )
+ {
+ rc = hvm_copy_to_guest_phys(last_addr + sizeof(start_info),
+ &vga_info, sizeof(vga_info), v);
+ if ( !rc )
+ {
+ start_info.version = 2;
+ start_info.vga_info.offset = sizeof(start_info);
+ start_info.vga_info.size = sizeof(vga_info);
+ }
+ else
+ printk("Unable to copy VGA info to guest\n");
+ }
+#endif
+
start_info.magic = XEN_HVM_START_MAGIC_VALUE;
start_info.flags = SIF_PRIVILEGED | SIF_INITDOMAIN;
rc = hvm_copy_to_guest_phys(last_addr, &start_info, sizeof(start_info), v);
--- 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
+ * | | struct hvm_start_info. Optional and only present in
+ * | | version 2 and newer of the structure when
+ * | | SIF_INITDOMAIN is set; zero if absent.
+ * 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. */
+ /*
+ * The following sub-structure is only present in version 2 and newer
+ * when SIF_INITDOMAIN is set. It is reserved in version 1 or when
+ * SIF_INITDOMAIN is clear, and absent in version 0.
+ */
+ struct { /* Coord-s of struct dom0_vga_console_info. */
+ uint16_t offset; /* ... from base of struct hvm_start_info. */
+ uint16_t size; /* ... of present parts of the struct. */
+ } vga_info;
};
struct hvm_modlist_entry {
On Tue, Sep 21, 2021 at 09:18:05AM +0200, Jan Beulich wrote:
> Like PV Dom0 in order to use the console if in a mode other than text
> 80x25 the kernel needs to be provided information about this mode. Bump
> HVM start info's "current" version to 2 and use a previously reserved
> 32-bit field to provide struct dom0_vga_console_info's position and
> size.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: New.
>
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -19,6 +19,7 @@
> */
>
> #include <xen/acpi.h>
> +#include <xen/console.h>
> #include <xen/init.h>
> #include <xen/libelf.h>
> #include <xen/multiboot.h>
> @@ -549,6 +550,11 @@ static int __init pvh_load_kernel(struct
> paddr_t last_addr;
> struct hvm_start_info start_info = { 0 };
> struct hvm_modlist_entry mod = { 0 };
> +#ifdef CONFIG_VIDEO
> + struct dom0_vga_console_info vga_info = { 0 };
> +#else
> + struct {} __maybe_unused vga_info;
> +#endif
> struct vcpu *v = d->vcpu[0];
> int rc;
>
> @@ -598,7 +604,7 @@ static int __init pvh_load_kernel(struct
> * split into smaller allocations, done as a single region in order to
> * simplify it.
> */
> - last_addr = find_memory(d, &elf, sizeof(start_info) +
> + last_addr = find_memory(d, &elf, sizeof(start_info) + sizeof(vga_info) +
> (initrd ? ROUNDUP(initrd->mod_end, PAGE_SIZE) +
> sizeof(mod)
> : 0) +
> @@ -672,6 +678,22 @@ static int __init pvh_load_kernel(struct
> last_addr += sizeof(mod);
> }
>
> +#ifdef CONFIG_VIDEO
> + if ( fill_console_start_info(&vga_info) )
> + {
> + rc = hvm_copy_to_guest_phys(last_addr + sizeof(start_info),
> + &vga_info, sizeof(vga_info), v);
> + if ( !rc )
> + {
> + start_info.version = 2;
> + start_info.vga_info.offset = sizeof(start_info);
> + start_info.vga_info.size = sizeof(vga_info);
> + }
> + else
> + printk("Unable to copy VGA info to guest\n");
> + }
> +#endif
> +
> start_info.magic = XEN_HVM_START_MAGIC_VALUE;
> start_info.flags = SIF_PRIVILEGED | SIF_INITDOMAIN;
> rc = hvm_copy_to_guest_phys(last_addr, &start_info, sizeof(start_info), v);
> --- 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).
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.
> + * | | 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.
> + * 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).
Thanks, Roger.
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
On 22/09/2021 16: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).
Absolutely correct. C is not an acceptable ABI description.
Furthermore, dom0_vga_console_info is a bad ABI to start with, as
demonstrated by the multiple problems we've had extending it in the past.
The MB1/2 framebuffer information would be a rather better example to
follow, but we'll surely need to pass the EDID string too (at least in
the case that there aren't EFI runtime services to use).
~Andrew
On 22.09.2021 19:03, Andrew Cooper wrote:
> On 22/09/2021 16: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).
>
> Absolutely correct. C is not an acceptable ABI description.
See my reply to Roger's earlier mail.
> Furthermore, dom0_vga_console_info is a bad ABI to start with, as
> demonstrated by the multiple problems we've had extending it in the past.
I don't view this as "problems", nor do I think we couldn't extend it
further that same way, if need be.
> The MB1/2 framebuffer information would be a rather better example to
> follow,
Maybe, but I'm not sure - it doesn't look any better extensibility-wise
than dom0_vga_console_info. Also MB1 doesn't really have separate
structures, so if anything it would need to be MB2.
> but we'll surely need to pass the EDID string too (at least in
> the case that there aren't EFI runtime services to use).
According to the understanding I've gained while putting together the
patch to retrieve EDID info when running under EFI, there's no way to
obtain these via runtime services. Yet I also don't see why we would
need to pass this here - we've got XENPF_firmware_info to retrieve
this data, and I didn't think we need PVH to behave differently from
PV in such regards.
Jan
© 2016 - 2024 Red Hat, Inc.