[PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Jan Beulich]

In order to try to debug hypervisor side breakage from XSA-378 I found
myself urged to finally give PVH Dom0 a try. Sadly things didn't work
quite as expected. In the course of investigating these issues I actually
spotted one piece of PV Dom0 breakage as well, a fix for which is also
included here.

There are two immediate remaining issues (also mentioned in affected
patches):

1) It is not clear to me how PCI device reporting is to work. PV Dom0
   reports devices as they're discovered, including ones the hypervisor
   may not have been able to discover itself (ones on segments other
   than 0 or hotplugged ones). The respective hypercall, however, is
   inaccessible to PVH Dom0. Depending on the answer to this, either
   the hypervisor will need changing (to permit the call) or patch 2
   here will need further refinement.

2) Dom0, unlike in the PV case, cannot access the screen (to use as a
   console) when in a non-default mode (i.e. not 80x25 text), as the
   necessary information (in particular about VESA-bases LFB modes) is
   not communicated. On the hypervisor side this looks like deliberate
   behavior, but it is unclear to me what the intentions were towards
   an alternative model. (X may be able to access the screen depending
   on whether it has a suitable driver besides the presently unusable
   /dev/fb<N> based one.)

1: xen/x86: prevent PVH type from getting clobbered
2: xen/x86: allow PVH Dom0 without XEN_PV=y
3: xen/x86: make "earlyprintk=xen" work better for PVH Dom0
4: xen/x86: allow "earlyprintk=xen" to work for PV Dom0
5: xen/x86: make "earlyprintk=xen" work for HVM/PVH DomU
6: xen/x86: generalize preferred console model from PV to PVH Dom0
7: xen/x86: hook up xen_banner() also for PVH 
8: x86/PVH: adjust function/data placement
9: xen/x86: adjust data placement

Jan

[PATCH 1/9] xen/x86: prevent PVH type from getting clobbered

[Jan Beulich]

Like xen_start_flags, xen_domain_type gets set before .bss gets cleared.
Hence this variable also needs to be prevented from getting put in .bss,
which is possible because XEN_NATIVE is an enumerator evaluating to
zero. Any use prior to init_hvm_pv_info() setting the variable again
would lead to wrong decisions; one such case is xenboot_console_setup()
when called as a result of "earlyprintk=xen".

Use __ro_after_init as more applicable than either __section(".data") or
__read_mostly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -52,9 +52,6 @@ DEFINE_PER_CPU(struct vcpu_info, xen_vcp
 DEFINE_PER_CPU(uint32_t, xen_vcpu_id);
 EXPORT_PER_CPU_SYMBOL(xen_vcpu_id);
 
-enum xen_domain_type xen_domain_type = XEN_NATIVE;
-EXPORT_SYMBOL_GPL(xen_domain_type);
-
 unsigned long *machine_to_phys_mapping = (void *)MACH2PHYS_VIRT_START;
 EXPORT_SYMBOL(machine_to_phys_mapping);
 unsigned long  machine_to_phys_nr;
@@ -69,9 +66,11 @@ __read_mostly int xen_have_vector_callba
 EXPORT_SYMBOL_GPL(xen_have_vector_callback);
 
 /*
- * NB: needs to live in .data because it's used by xen_prepare_pvh which runs
- * before clearing the bss.
+ * NB: These need to live in .data or alike because they're used by
+ * xen_prepare_pvh() which runs before clearing the bss.
  */
+enum xen_domain_type __ro_after_init xen_domain_type = XEN_NATIVE;
+EXPORT_SYMBOL_GPL(xen_domain_type);
 uint32_t xen_start_flags __section(".data") = 0;
 EXPORT_SYMBOL(xen_start_flags);
 

Re: [PATCH 1/9] xen/x86: prevent PVH type from getting clobbered

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 671 bytes --]

On 07.09.21 12:07, Jan Beulich wrote:
> Like xen_start_flags, xen_domain_type gets set before .bss gets cleared.
> Hence this variable also needs to be prevented from getting put in .bss,
> which is possible because XEN_NATIVE is an enumerator evaluating to
> zero. Any use prior to init_hvm_pv_info() setting the variable again
> would lead to wrong decisions; one such case is xenboot_console_setup()
> when called as a result of "earlyprintk=xen".
> 
> Use __ro_after_init as more applicable than either __section(".data") or
> __read_mostly.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 2/9] xen/x86: allow PVH Dom0 without XEN_PV=y

[Jan Beulich]

Decouple XEN_DOM0 from XEN_PV, converting some existing uses of XEN_DOM0
to a new XEN_PV_DOM0. (I'm not convinced all are really / should really
be PV-specific, but for starters I've tried to be conservative.)

For PVH Dom0 the hypervisor populates MADT with only x2APIC entries, so
without x2APIC support enabled in the kernel things aren't going to work
very well. (As opposed, DomU-s would only ever see LAPIC entries in MADT
as of now.) Note that this then requires PVH Dom0 to be 64-bit, as
X86_X2APIC depends on X86_64.

In the course of this xen_running_on_version_or_later() needs to be
available more broadly. Move it from a PV-specific to a generic file,
considering that what it does isn't really PV-specific at all anyway.

Note that xen/interface/version.h cannot be included on its own; in
enlighten.c, which uses SCHEDOP_* anyway, include xen/interface/sched.h
first to resolve the apparently sole missing type (xen_ulong_t).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
For drivers/xen/pci.c it's not clear to me what the intentions are: On
one hand I would think this is needed in PVH as well (especially for
hotplugged devices), yet otoh the hypervisor's hvm_physdev_op() doesn't
let the respective physdev-ops through. As a result with how I have
things, a lot of "Failed to add - passthrough or MSI/MSI-X might fail!"
can be observed.

For arch/x86/xen/vga.c I think it really is needed for PVH Dom0 as well,
except that the needed data doesn't get supplied yet by the hypervisor
afaict.

--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -10,6 +10,8 @@
 
 #include <xen/xen.h>
 #include <xen/features.h>
+#include <xen/interface/sched.h>
+#include <xen/interface/version.h>
 #include <xen/page.h>
 
 #include <asm/xen/hypercall.h>
@@ -257,6 +259,21 @@ int xen_vcpu_setup(int cpu)
 	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
 }
 
+/* Check if running on Xen version (major, minor) or later */
+bool xen_running_on_version_or_later(unsigned int major, unsigned int minor)
+{
+	unsigned int version;
+
+	if (!xen_domain())
+		return false;
+
+	version = HYPERVISOR_xen_version(XENVER_version, NULL);
+	if ((((version >> 16) == major) && ((version & 0xffff) >= minor)) ||
+		((version >> 16) > major))
+		return true;
+	return false;
+}
+
 void xen_reboot(int reason)
 {
 	struct sched_shutdown r = { .reason = reason };
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -142,22 +142,6 @@ static void __init xen_pv_guest_late_ini
 #endif
 }
 
-/* Check if running on Xen version (major, minor) or later */
-bool
-xen_running_on_version_or_later(unsigned int major, unsigned int minor)
-{
-	unsigned int version;
-
-	if (!xen_domain())
-		return false;
-
-	version = HYPERVISOR_xen_version(XENVER_version, NULL);
-	if ((((version >> 16) == major) && ((version & 0xffff) >= minor)) ||
-		((version >> 16) > major))
-		return true;
-	return false;
-}
-
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -14,16 +14,19 @@ static inline int pci_xen_hvm_init(void)
 	return -1;
 }
 #endif
-#if defined(CONFIG_XEN_DOM0)
+#ifdef CONFIG_XEN_PV_DOM0
 int __init pci_xen_initial_domain(void);
-int xen_find_device_domain_owner(struct pci_dev *dev);
-int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
-int xen_unregister_device_domain_owner(struct pci_dev *dev);
 #else
 static inline int __init pci_xen_initial_domain(void)
 {
 	return -1;
 }
+#endif
+#ifdef CONFIG_XEN_DOM0
+int xen_find_device_domain_owner(struct pci_dev *dev);
+int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
+int xen_unregister_device_domain_owner(struct pci_dev *dev);
+#else
 static inline int xen_find_device_domain_owner(struct pci_dev *dev)
 {
 	return -1;
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -113,7 +113,7 @@ static int acpi_register_gsi_xen_hvm(str
 				 false /* no mapping of GSI to PIRQ */);
 }
 
-#ifdef CONFIG_XEN_DOM0
+#ifdef CONFIG_XEN_PV_DOM0
 static int xen_register_gsi(u32 gsi, int triggering, int polarity)
 {
 	int rc, irq;
@@ -261,7 +261,7 @@ error:
 	return irq;
 }
 
-#ifdef CONFIG_XEN_DOM0
+#ifdef CONFIG_XEN_PV_DOM0
 static bool __read_mostly pci_seg_supported = true;
 
 static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
@@ -375,10 +375,10 @@ static void xen_initdom_restore_msi_irqs
 		WARN(ret && ret != -ENOSYS, "restore_msi -> %d\n", ret);
 	}
 }
-#else /* CONFIG_XEN_DOM0 */
+#else /* CONFIG_XEN_PV_DOM0 */
 #define xen_initdom_setup_msi_irqs	NULL
 #define xen_initdom_restore_msi_irqs	NULL
-#endif /* !CONFIG_XEN_DOM0 */
+#endif /* !CONFIG_XEN_PV_DOM0 */
 
 static void xen_teardown_msi_irqs(struct pci_dev *dev)
 {
@@ -555,7 +555,7 @@ int __init pci_xen_hvm_init(void)
 	return 0;
 }
 
-#ifdef CONFIG_XEN_DOM0
+#ifdef CONFIG_XEN_PV_DOM0
 int __init pci_xen_initial_domain(void)
 {
 	int irq;
@@ -583,6 +583,9 @@ int __init pci_xen_initial_domain(void)
 	}
 	return 0;
 }
+#endif
+
+#ifdef CONFIG_XEN_DOM0
 
 struct xen_device_domain_owner {
 	domid_t domain;
@@ -656,4 +659,4 @@ int xen_unregister_device_domain_owner(s
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xen_unregister_device_domain_owner);
-#endif
+#endif /* CONFIG_XEN_DOM0 */
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -43,13 +43,9 @@ config XEN_PV_SMP
 	def_bool y
 	depends on XEN_PV && SMP
 
-config XEN_DOM0
-	bool "Xen PV Dom0 support"
-	default y
-	depends on XEN_PV && PCI_XEN && SWIOTLB_XEN
-	depends on X86_IO_APIC && ACPI && PCI
-	help
-	  Support running as a Xen PV Dom0 guest.
+config XEN_PV_DOM0
+	def_bool y
+	depends on XEN_PV && XEN_DOM0
 
 config XEN_PVHVM
 	def_bool y
@@ -86,3 +82,12 @@ config XEN_PVH
 	def_bool n
 	help
 	  Support for running as a Xen PVH guest.
+
+config XEN_DOM0
+	bool "Xen Dom0 support"
+	default XEN_PV
+	depends on (XEN_PV && SWIOTLB_XEN) || (XEN_PVH && X86_64)
+	depends on X86_IO_APIC && ACPI && PCI
+	select X86_X2APIC if XEN_PVH && X86_64
+	help
+	  Support running as a Xen Dom0 guest.
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -45,7 +45,7 @@ obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= spinl
 
 obj-$(CONFIG_XEN_DEBUG_FS)	+= debugfs.o
 
-obj-$(CONFIG_XEN_DOM0)		+= vga.o
+obj-$(CONFIG_XEN_PV_DOM0)	+= vga.o
 
 obj-$(CONFIG_SWIOTLB_XEN)	+= pci-swiotlb-xen.o
 
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -109,7 +109,7 @@ static inline void xen_uninit_lock_cpu(i
 
 struct dom0_vga_console_info;
 
-#ifdef CONFIG_XEN_DOM0
+#ifdef CONFIG_XEN_PV_DOM0
 void __init xen_init_vga(const struct dom0_vga_console_info *, size_t size);
 #else
 static inline void __init xen_init_vga(const struct dom0_vga_console_info *info,
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -240,7 +240,7 @@ config XEN_PRIVCMD
 
 config XEN_ACPI_PROCESSOR
 	tristate "Xen ACPI processor"
-	depends on XEN && XEN_DOM0 && X86 && ACPI_PROCESSOR && CPU_FREQ
+	depends on XEN && XEN_PV_DOM0 && X86 && ACPI_PROCESSOR && CPU_FREQ
 	default m
 	help
 	  This ACPI processor uploads Power Management information to the Xen
@@ -258,7 +258,7 @@ config XEN_ACPI_PROCESSOR
 
 config XEN_MCE_LOG
 	bool "Xen platform mcelog"
-	depends on XEN_DOM0 && X86_MCE
+	depends on XEN_PV_DOM0 && X86_MCE
 	help
 	  Allow kernel fetching MCE error from Xen platform and
 	  converting it into Linux mcelog format for mcelog tools

Re: [PATCH 2/9] xen/x86: allow PVH Dom0 without XEN_PV=y

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1146 bytes --]

On 07.09.21 12:08, Jan Beulich wrote:
> Decouple XEN_DOM0 from XEN_PV, converting some existing uses of XEN_DOM0
> to a new XEN_PV_DOM0. (I'm not convinced all are really / should really
> be PV-specific, but for starters I've tried to be conservative.)
> 
> For PVH Dom0 the hypervisor populates MADT with only x2APIC entries, so
> without x2APIC support enabled in the kernel things aren't going to work
> very well. (As opposed, DomU-s would only ever see LAPIC entries in MADT
> as of now.) Note that this then requires PVH Dom0 to be 64-bit, as
> X86_X2APIC depends on X86_64.
> 
> In the course of this xen_running_on_version_or_later() needs to be
> available more broadly. Move it from a PV-specific to a generic file,
> considering that what it does isn't really PV-specific at all anyway.
> 
> Note that xen/interface/version.h cannot be included on its own; in
> enlighten.c, which uses SCHEDOP_* anyway, include xen/interface/sched.h
> first to resolve the apparently sole missing type (xen_ulong_t).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 3/9] xen/x86: make "earlyprintk=xen" work better for PVH Dom0

[Jan Beulich]

The xen_hvm_early_write() path better wouldn't be taken in this case;
while port 0xE9 can be used, the hypercall path is quite a bit more
efficient. Put that first, as it may also work for DomU-s (see also
xen_raw_console_write()).

While there also bail from the function when the first
domU_write_console() failed - later ones aren't going to succeed.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -632,17 +632,16 @@ static void xenboot_write_console(struct
 	unsigned int linelen, off = 0;
 	const char *pos;
 
+	if (dom0_write_console(0, string, len) >= 0)
+		return;
+
 	if (!xen_pv_domain()) {
 		xen_hvm_early_write(0, string, len);
 		return;
 	}
 
-	dom0_write_console(0, string, len);
-
-	if (xen_initial_domain())
+	if (domU_write_console(0, "(early) ", 8) < 0)
 		return;
-
-	domU_write_console(0, "(early) ", 8);
 	while (off < len && NULL != (pos = strchr(string+off, '\n'))) {
 		linelen = pos-string+off;
 		if (off + linelen > len)

Re: [PATCH 3/9] xen/x86: make "earlyprintk=xen" work better for PVH Dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 529 bytes --]

On 07.09.21 12:09, Jan Beulich wrote:
> The xen_hvm_early_write() path better wouldn't be taken in this case;
> while port 0xE9 can be used, the hypercall path is quite a bit more
> efficient. Put that first, as it may also work for DomU-s (see also
> xen_raw_console_write()).
> 
> While there also bail from the function when the first
> domU_write_console() failed - later ones aren't going to succeed.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 4/9] xen/x86: allow "earlyprintk=xen" to work for PV Dom0

[Jan Beulich]

With preferred consoles "tty" and "hvc" announced as preferred,
registering "xenboot" early won't result in use of the console: It also
needs to be registered as preferred. Generalize this from being DomU-
only so far.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1340,7 +1340,6 @@ asmlinkage __visible void __init xen_sta
 	boot_params.hdr.hardware_subarch = X86_SUBARCH_XEN;
 
 	if (!xen_initial_domain()) {
-		add_preferred_console("xenboot", 0, NULL);
 		if (pci_xen)
 			x86_init.pci.arch_init = pci_xen_init;
 	} else {
@@ -1383,6 +1382,7 @@ asmlinkage __visible void __init xen_sta
 #endif
 	}
 
+	add_preferred_console("xenboot", 0, NULL);
 	if (!boot_params.screen_info.orig_video_isVGA)
 		add_preferred_console("tty", 0, NULL);
 	add_preferred_console("hvc", 0, NULL);

Re: [PATCH 4/9] xen/x86: allow "earlyprintk=xen" to work for PV Dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 385 bytes --]

On 07.09.21 12:09, Jan Beulich wrote:
> With preferred consoles "tty" and "hvc" announced as preferred,
> registering "xenboot" early won't result in use of the console: It also
> needs to be registered as preferred. Generalize this from being DomU-
> only so far.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 5/9] xen/x86: make "earlyprintk=xen" work for HVM/PVH DomU

[Jan Beulich]

xenboot_write_console() is dealing with these quite fine so I don't see
why xenboot_console_setup() would return -ENOENT in this case.

Adjust documentation accordingly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1266,7 +1266,7 @@
 			The VGA and EFI output is eventually overwritten by
 			the real console.
 
-			The xen output can only be used by Xen PV guests.
+			The xen option can only be used in Xen domains.
 
 			The sclp output can only be used on s390.
 
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -618,10 +618,8 @@ static int __init xenboot_console_setup(
 {
 	static struct xencons_info xenboot;
 
-	if (xen_initial_domain())
+	if (xen_initial_domain() || !xen_pv_domain())
 		return 0;
-	if (!xen_pv_domain())
-		return -ENODEV;
 
 	return xencons_info_pv_init(&xenboot, 0);
 }

Re: [PATCH 5/9] xen/x86: make "earlyprintk=xen" work for HVM/PVH DomU

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 336 bytes --]

On 07.09.21 12:10, Jan Beulich wrote:
> xenboot_write_console() is dealing with these quite fine so I don't see
> why xenboot_console_setup() would return -ENOENT in this case.
> 
> Adjust documentation accordingly.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 6/9] xen/x86: generalize preferred console model from PV to PVH Dom0

[Jan Beulich]

Without announcing hvc0 as preferred it won't get used as long as tty0
gets registered earlier. This is particularly problematic with there not
being any screen output for PVH Dom0 when the screen is in graphics
mode, as the necessary information doesn't get conveyed yet from the
hypervisor.

Follow PV's model, but be conservative and do this for Dom0 only for
now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Prior to 418492ba40b2 ("x86/virt/xen: Use guest_late_init to detect Xen
PVH guest") x86_init.oem.arch_setup was already used by PVH, so I assume
the use of this hook is acceptable here.

Seeing that change, I wonder in how far setting xen_pvh to true only in
xen_hvm_guest_late_init() can really work: This hook, as its name says,
gets called pretty late; any decision taken earlier might have been
wrong. One such wrong decision is what gets added here - preferred
consoles won't be registered when taking that path. While adding a 2nd
call there might work, aiui they would better be registered prior to
parse_early_param(), i.e. before "earlyprintk=" gets evaluated.

I also consider tying "detecting" PVH mode to the no-VGA and no-CMOS-RTC
FADT flags as problematic looking forward: There may conceivably be
"legacy free" HVM guests down the road, yet they shouldn't be mistaken
for being PVH. Most of the XEN_X86_EMU_* controlled functionality would
seem unsuitable for the same reason; presence/absence of
XENFEAT_hvm_pirqs (tied to XEN_X86_EMU_USE_PIRQ) might be sufficiently
reliable an indicator. Question there is whether the separation
introduced by Xen commit b96b50004804 ("x86: remove XENFEAT_hvm_pirqs
for PVHv2 guests") came early enough in the process of enabling PVHv2.
Plus I'm not sure a HVM guest without pass-through enabled couldn't be
run with this off (i.e. by relaxing emulation_flags_ok() and having the
tool stack not request this emulation in such cases).

I think the approach here might be equally applicable for DomU, albeit
potentially pointless (i.e. dropping the conditional might make sense
even if simply benign there): A PVH DomU ought to never come with a VGA
console. Yet even then a dummy one may still get registered and would
take precedence over hvc?

--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -3,6 +3,7 @@
 #ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
 #include <linux/memblock.h>
 #endif
+#include <linux/console.h>
 #include <linux/cpu.h>
 #include <linux/kexec.h>
 #include <linux/slab.h>
@@ -18,6 +19,7 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/cpu.h>
 #include <asm/e820/api.h> 
+#include <asm/setup.h>
 
 #include "xen-ops.h"
 #include "smp.h"
@@ -274,6 +276,16 @@ bool xen_running_on_version_or_later(uns
 	return false;
 }
 
+void __init xen_add_preferred_consoles(void)
+{
+	add_preferred_console("xenboot", 0, NULL);
+	if (!boot_params.screen_info.orig_video_isVGA)
+		add_preferred_console("tty", 0, NULL);
+	add_preferred_console("hvc", 0, NULL);
+	if (boot_params.screen_info.orig_video_isVGA)
+		add_preferred_console("tty", 0, NULL);
+}
+
 void xen_reboot(int reason)
 {
 	struct sched_shutdown r = { .reason = reason };
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -28,7 +28,6 @@
 #include <linux/mm.h>
 #include <linux/page-flags.h>
 #include <linux/highmem.h>
-#include <linux/console.h>
 #include <linux/pci.h>
 #include <linux/gfp.h>
 #include <linux/edd.h>
@@ -1382,12 +1381,7 @@ asmlinkage __visible void __init xen_sta
 #endif
 	}
 
-	add_preferred_console("xenboot", 0, NULL);
-	if (!boot_params.screen_info.orig_video_isVGA)
-		add_preferred_console("tty", 0, NULL);
-	add_preferred_console("hvc", 0, NULL);
-	if (boot_params.screen_info.orig_video_isVGA)
-		add_preferred_console("tty", 0, NULL);
+	xen_add_preferred_consoles();
 
 #ifdef CONFIG_PCI
 	/* PCI BIOS service won't work from a PV guest. */
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -36,6 +36,9 @@ void __init xen_pvh_init(struct boot_par
 	pfn = __pa(hypercall_page);
 	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
 
+	if (xen_initial_domain())
+		x86_init.oem.arch_setup = xen_add_preferred_consoles;
+
 	xen_efi_init(boot_params);
 }
 
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -118,6 +118,8 @@ static inline void __init xen_init_vga(c
 }
 #endif
 
+void xen_add_preferred_consoles(void);
+
 void __init xen_init_apic(void);
 
 #ifdef CONFIG_XEN_EFI

Re: [PATCH 6/9] xen/x86: generalize preferred console model from PV to PVH Dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2059 bytes --]

On 07.09.21 12:10, Jan Beulich wrote:
> Without announcing hvc0 as preferred it won't get used as long as tty0
> gets registered earlier. This is particularly problematic with there not
> being any screen output for PVH Dom0 when the screen is in graphics
> mode, as the necessary information doesn't get conveyed yet from the
> hypervisor.
> 
> Follow PV's model, but be conservative and do this for Dom0 only for
> now.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> Prior to 418492ba40b2 ("x86/virt/xen: Use guest_late_init to detect Xen
> PVH guest") x86_init.oem.arch_setup was already used by PVH, so I assume
> the use of this hook is acceptable here.

Yes, I think so.

> Seeing that change, I wonder in how far setting xen_pvh to true only in
> xen_hvm_guest_late_init() can really work: This hook, as its name says,
> gets called pretty late; any decision taken earlier might have been
> wrong. One such wrong decision is what gets added here - preferred
> consoles won't be registered when taking that path. While adding a 2nd
> call there might work, aiui they would better be registered prior to
> parse_early_param(), i.e. before "earlyprintk=" gets evaluated.
> 
> I also consider tying "detecting" PVH mode to the no-VGA and no-CMOS-RTC
> FADT flags as problematic looking forward: There may conceivably be
> "legacy free" HVM guests down the road, yet they shouldn't be mistaken
> for being PVH. Most of the XEN_X86_EMU_* controlled functionality would
> seem unsuitable for the same reason; presence/absence of
> XENFEAT_hvm_pirqs (tied to XEN_X86_EMU_USE_PIRQ) might be sufficiently
> reliable an indicator. Question there is whether the separation
> introduced by Xen commit b96b50004804 ("x86: remove XENFEAT_hvm_pirqs
> for PVHv2 guests") came early enough in the process of enabling PVHv2.

Yes, it did. The boot path not using the PVH specific entry point was
enabled with Xen 4.11, while commit b96b50004804 was in 4.9.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Jan Beulich]

This was effectively lost while dropping PVHv1 code. Move the function
and arrange for it to be called the same way as done in PV mode. Clearly
this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
check that was recently removed, as that's a PV-only feature.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
 	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
 }
 
+void __init xen_banner(void)
+{
+	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
+	struct xen_extraversion extra;
+	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
+
+	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
+	pr_info("Xen version: %u.%u%s%s\n",
+		version >> 16, version & 0xffff, extra.extraversion,
+		xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " (preserve-AD)" : "");
+}
+
 /* Check if running on Xen version (major, minor) or later */
 bool xen_running_on_version_or_later(unsigned int major, unsigned int minor)
 {
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -108,17 +108,6 @@ struct tls_descs {
  */
 static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
 
-static void __init xen_banner(void)
-{
-	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
-	struct xen_extraversion extra;
-	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
-
-	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
-	pr_info("Xen version: %d.%d%s (preserve-AD)\n",
-		version >> 16, version & 0xffff, extra.extraversion);
-}
-
 static void __init xen_pv_init_platform(void)
 {
 	populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -38,6 +38,7 @@ void __init xen_pvh_init(struct boot_par
 
 	if (xen_initial_domain())
 		x86_init.oem.arch_setup = xen_add_preferred_consoles;
+	x86_init.oem.banner = xen_banner;
 
 	xen_efi_init(boot_params);
 }
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -51,6 +51,7 @@ void __init xen_remap_memory(void);
 phys_addr_t __init xen_find_free_area(phys_addr_t size);
 char * __init xen_memory_setup(void);
 void __init xen_arch_setup(void);
+void xen_banner(void);
 void xen_enable_sysenter(void);
 void xen_enable_syscall(void);
 void xen_vcpu_restore(void);

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1034 bytes --]

On 07.09.21 12:11, Jan Beulich wrote:
> This was effectively lost while dropping PVHv1 code. Move the function
> and arrange for it to be called the same way as done in PV mode. Clearly
> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
> check that was recently removed, as that's a PV-only feature.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>   	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>   }
>   
> +void __init xen_banner(void)
> +{
> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
> +	struct xen_extraversion extra;

Please add a blank line here.

> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
> +
> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);

Is this correct? I don't think the kernel needs to be paravirtualized
with PVH (at least not to the same extend as for PV).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Jan Beulich]

On 23.09.2021 16:59, Juergen Gross wrote:
> On 07.09.21 12:11, Jan Beulich wrote:
>> This was effectively lost while dropping PVHv1 code. Move the function
>> and arrange for it to be called the same way as done in PV mode. Clearly
>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>> check that was recently removed, as that's a PV-only feature.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>   	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>   }
>>   
>> +void __init xen_banner(void)
>> +{
>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>> +	struct xen_extraversion extra;
> 
> Please add a blank line here.

Oops.

>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>> +
>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
> 
> Is this correct? I don't think the kernel needs to be paravirtualized
> with PVH (at least not to the same extend as for PV).

What else do you suggest the message to say? Simply drop
"paravirtualized"? To some extent it is applicable imo, further
qualified by pv_info.name. And that's how it apparently was with
PVHv1.

Jan

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1459 bytes --]

On 23.09.21 17:10, Jan Beulich wrote:
> On 23.09.2021 16:59, Juergen Gross wrote:
>> On 07.09.21 12:11, Jan Beulich wrote:
>>> This was effectively lost while dropping PVHv1 code. Move the function
>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>> check that was recently removed, as that's a PV-only feature.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/arch/x86/xen/enlighten.c
>>> +++ b/arch/x86/xen/enlighten.c
>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>    	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>    }
>>>    
>>> +void __init xen_banner(void)
>>> +{
>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>> +	struct xen_extraversion extra;
>>
>> Please add a blank line here.
> 
> Oops.
> 
>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>> +
>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>
>> Is this correct? I don't think the kernel needs to be paravirtualized
>> with PVH (at least not to the same extend as for PV).
> 
> What else do you suggest the message to say? Simply drop
> "paravirtualized"? To some extent it is applicable imo, further
> qualified by pv_info.name. And that's how it apparently was with
> PVHv1.

The string could be selected depending on CONFIG_XEN_PV.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Jan Beulich]

On 23.09.2021 17:15, Juergen Gross wrote:
> On 23.09.21 17:10, Jan Beulich wrote:
>> On 23.09.2021 16:59, Juergen Gross wrote:
>>> On 07.09.21 12:11, Jan Beulich wrote:
>>>> This was effectively lost while dropping PVHv1 code. Move the function
>>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>>> check that was recently removed, as that's a PV-only feature.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/arch/x86/xen/enlighten.c
>>>> +++ b/arch/x86/xen/enlighten.c
>>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>>    	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>>    }
>>>>    
>>>> +void __init xen_banner(void)
>>>> +{
>>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>>> +	struct xen_extraversion extra;
>>>
>>> Please add a blank line here.
>>
>> Oops.
>>
>>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>>> +
>>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>>
>>> Is this correct? I don't think the kernel needs to be paravirtualized
>>> with PVH (at least not to the same extend as for PV).
>>
>> What else do you suggest the message to say? Simply drop
>> "paravirtualized"? To some extent it is applicable imo, further
>> qualified by pv_info.name. And that's how it apparently was with
>> PVHv1.
> 
> The string could be selected depending on CONFIG_XEN_PV.

Hmm, now I'm confused: Doesn't this setting control whether the kernel
can run in PV mode? If so, that functionality being present should have
no effect on the functionality of the kernel when running in PVH mode.
So what you suggest would end up in misleading information imo.

Jan

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2045 bytes --]

On 23.09.21 17:19, Jan Beulich wrote:
> On 23.09.2021 17:15, Juergen Gross wrote:
>> On 23.09.21 17:10, Jan Beulich wrote:
>>> On 23.09.2021 16:59, Juergen Gross wrote:
>>>> On 07.09.21 12:11, Jan Beulich wrote:
>>>>> This was effectively lost while dropping PVHv1 code. Move the function
>>>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>>>> check that was recently removed, as that's a PV-only feature.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/arch/x86/xen/enlighten.c
>>>>> +++ b/arch/x86/xen/enlighten.c
>>>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>>>     	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>>>     }
>>>>>     
>>>>> +void __init xen_banner(void)
>>>>> +{
>>>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>>>> +	struct xen_extraversion extra;
>>>>
>>>> Please add a blank line here.
>>>
>>> Oops.
>>>
>>>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>>>> +
>>>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>>>
>>>> Is this correct? I don't think the kernel needs to be paravirtualized
>>>> with PVH (at least not to the same extend as for PV).
>>>
>>> What else do you suggest the message to say? Simply drop
>>> "paravirtualized"? To some extent it is applicable imo, further
>>> qualified by pv_info.name. And that's how it apparently was with
>>> PVHv1.
>>
>> The string could be selected depending on CONFIG_XEN_PV.
> 
> Hmm, now I'm confused: Doesn't this setting control whether the kernel
> can run in PV mode? If so, that functionality being present should have
> no effect on the functionality of the kernel when running in PVH mode.
> So what you suggest would end up in misleading information imo.

Hmm, yes, I mixed "paravirtualized" with "capable to run
paravirtualized".

So the string should depend on xen_pv_domain().


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Jan Beulich]

On 23.09.2021 17:25, Juergen Gross wrote:
> On 23.09.21 17:19, Jan Beulich wrote:
>> On 23.09.2021 17:15, Juergen Gross wrote:
>>> On 23.09.21 17:10, Jan Beulich wrote:
>>>> On 23.09.2021 16:59, Juergen Gross wrote:
>>>>> On 07.09.21 12:11, Jan Beulich wrote:
>>>>>> This was effectively lost while dropping PVHv1 code. Move the function
>>>>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>>>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>>>>> check that was recently removed, as that's a PV-only feature.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>> --- a/arch/x86/xen/enlighten.c
>>>>>> +++ b/arch/x86/xen/enlighten.c
>>>>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>>>>     	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>>>>     }
>>>>>>     
>>>>>> +void __init xen_banner(void)
>>>>>> +{
>>>>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>>>>> +	struct xen_extraversion extra;
>>>>>
>>>>> Please add a blank line here.
>>>>
>>>> Oops.
>>>>
>>>>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>>>>> +
>>>>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>>>>
>>>>> Is this correct? I don't think the kernel needs to be paravirtualized
>>>>> with PVH (at least not to the same extend as for PV).
>>>>
>>>> What else do you suggest the message to say? Simply drop
>>>> "paravirtualized"? To some extent it is applicable imo, further
>>>> qualified by pv_info.name. And that's how it apparently was with
>>>> PVHv1.
>>>
>>> The string could be selected depending on CONFIG_XEN_PV.
>>
>> Hmm, now I'm confused: Doesn't this setting control whether the kernel
>> can run in PV mode? If so, that functionality being present should have
>> no effect on the functionality of the kernel when running in PVH mode.
>> So what you suggest would end up in misleading information imo.
> 
> Hmm, yes, I mixed "paravirtualized" with "capable to run
> paravirtualized".
> 
> So the string should depend on xen_pv_domain().

But that's already expressed by pv_info.name then being "Xen PV".

Jan

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2370 bytes --]

On 23.09.21 17:31, Jan Beulich wrote:
> On 23.09.2021 17:25, Juergen Gross wrote:
>> On 23.09.21 17:19, Jan Beulich wrote:
>>> On 23.09.2021 17:15, Juergen Gross wrote:
>>>> On 23.09.21 17:10, Jan Beulich wrote:
>>>>> On 23.09.2021 16:59, Juergen Gross wrote:
>>>>>> On 07.09.21 12:11, Jan Beulich wrote:
>>>>>>> This was effectively lost while dropping PVHv1 code. Move the function
>>>>>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>>>>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>>>>>> check that was recently removed, as that's a PV-only feature.
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>
>>>>>>> --- a/arch/x86/xen/enlighten.c
>>>>>>> +++ b/arch/x86/xen/enlighten.c
>>>>>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>>>>>      	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>>>>>      }
>>>>>>>      
>>>>>>> +void __init xen_banner(void)
>>>>>>> +{
>>>>>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>>>>>> +	struct xen_extraversion extra;
>>>>>>
>>>>>> Please add a blank line here.
>>>>>
>>>>> Oops.
>>>>>
>>>>>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>>>>>> +
>>>>>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>>>>>
>>>>>> Is this correct? I don't think the kernel needs to be paravirtualized
>>>>>> with PVH (at least not to the same extend as for PV).
>>>>>
>>>>> What else do you suggest the message to say? Simply drop
>>>>> "paravirtualized"? To some extent it is applicable imo, further
>>>>> qualified by pv_info.name. And that's how it apparently was with
>>>>> PVHv1.
>>>>
>>>> The string could be selected depending on CONFIG_XEN_PV.
>>>
>>> Hmm, now I'm confused: Doesn't this setting control whether the kernel
>>> can run in PV mode? If so, that functionality being present should have
>>> no effect on the functionality of the kernel when running in PVH mode.
>>> So what you suggest would end up in misleading information imo.
>>
>> Hmm, yes, I mixed "paravirtualized" with "capable to run
>> paravirtualized".
>>
>> So the string should depend on xen_pv_domain().
> 
> But that's already expressed by pv_info.name then being "Xen PV".

True. Okay, I'm fine with just dropping "paravirtualized".


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Jan Beulich]

On 29.09.2021 07:45, Juergen Gross wrote:
> On 23.09.21 17:31, Jan Beulich wrote:
>> On 23.09.2021 17:25, Juergen Gross wrote:
>>> On 23.09.21 17:19, Jan Beulich wrote:
>>>> On 23.09.2021 17:15, Juergen Gross wrote:
>>>>> On 23.09.21 17:10, Jan Beulich wrote:
>>>>>> On 23.09.2021 16:59, Juergen Gross wrote:
>>>>>>> On 07.09.21 12:11, Jan Beulich wrote:
>>>>>>>> This was effectively lost while dropping PVHv1 code. Move the function
>>>>>>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>>>>>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>>>>>>> check that was recently removed, as that's a PV-only feature.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>>
>>>>>>>> --- a/arch/x86/xen/enlighten.c
>>>>>>>> +++ b/arch/x86/xen/enlighten.c
>>>>>>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>>>>>>      	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>>>>>>      }
>>>>>>>>      
>>>>>>>> +void __init xen_banner(void)
>>>>>>>> +{
>>>>>>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>>>>>>> +	struct xen_extraversion extra;
>>>>>>>
>>>>>>> Please add a blank line here.
>>>>>>
>>>>>> Oops.
>>>>>>
>>>>>>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>>>>>>> +
>>>>>>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>>>>>>
>>>>>>> Is this correct? I don't think the kernel needs to be paravirtualized
>>>>>>> with PVH (at least not to the same extend as for PV).
>>>>>>
>>>>>> What else do you suggest the message to say? Simply drop
>>>>>> "paravirtualized"? To some extent it is applicable imo, further
>>>>>> qualified by pv_info.name. And that's how it apparently was with
>>>>>> PVHv1.
>>>>>
>>>>> The string could be selected depending on CONFIG_XEN_PV.
>>>>
>>>> Hmm, now I'm confused: Doesn't this setting control whether the kernel
>>>> can run in PV mode? If so, that functionality being present should have
>>>> no effect on the functionality of the kernel when running in PVH mode.
>>>> So what you suggest would end up in misleading information imo.
>>>
>>> Hmm, yes, I mixed "paravirtualized" with "capable to run
>>> paravirtualized".
>>>
>>> So the string should depend on xen_pv_domain().
>>
>> But that's already expressed by pv_info.name then being "Xen PV".
> 
> True. Okay, I'm fine with just dropping "paravirtualized".

Done. Do you want me to also make pr_info() vs printk(KERN_INFO ...)
consistent in the function at this occasion? If so - which of the two?

Jan

Re: [PATCH 7/9] xen/x86: hook up xen_banner() also for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2744 bytes --]

On 29.09.21 09:28, Jan Beulich wrote:
> On 29.09.2021 07:45, Juergen Gross wrote:
>> On 23.09.21 17:31, Jan Beulich wrote:
>>> On 23.09.2021 17:25, Juergen Gross wrote:
>>>> On 23.09.21 17:19, Jan Beulich wrote:
>>>>> On 23.09.2021 17:15, Juergen Gross wrote:
>>>>>> On 23.09.21 17:10, Jan Beulich wrote:
>>>>>>> On 23.09.2021 16:59, Juergen Gross wrote:
>>>>>>>> On 07.09.21 12:11, Jan Beulich wrote:
>>>>>>>>> This was effectively lost while dropping PVHv1 code. Move the function
>>>>>>>>> and arrange for it to be called the same way as done in PV mode. Clearly
>>>>>>>>> this then needs re-introducing the XENFEAT_mmu_pt_update_preserve_ad
>>>>>>>>> check that was recently removed, as that's a PV-only feature.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>
>>>>>>>>> --- a/arch/x86/xen/enlighten.c
>>>>>>>>> +++ b/arch/x86/xen/enlighten.c
>>>>>>>>> @@ -261,6 +261,18 @@ int xen_vcpu_setup(int cpu)
>>>>>>>>>       	return ((per_cpu(xen_vcpu, cpu) == NULL) ? -ENODEV : 0);
>>>>>>>>>       }
>>>>>>>>>       
>>>>>>>>> +void __init xen_banner(void)
>>>>>>>>> +{
>>>>>>>>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>>>>>>>> +	struct xen_extraversion extra;
>>>>>>>>
>>>>>>>> Please add a blank line here.
>>>>>>>
>>>>>>> Oops.
>>>>>>>
>>>>>>>>> +	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>>>>>>>>> +
>>>>>>>>> +	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
>>>>>>>>
>>>>>>>> Is this correct? I don't think the kernel needs to be paravirtualized
>>>>>>>> with PVH (at least not to the same extend as for PV).
>>>>>>>
>>>>>>> What else do you suggest the message to say? Simply drop
>>>>>>> "paravirtualized"? To some extent it is applicable imo, further
>>>>>>> qualified by pv_info.name. And that's how it apparently was with
>>>>>>> PVHv1.
>>>>>>
>>>>>> The string could be selected depending on CONFIG_XEN_PV.
>>>>>
>>>>> Hmm, now I'm confused: Doesn't this setting control whether the kernel
>>>>> can run in PV mode? If so, that functionality being present should have
>>>>> no effect on the functionality of the kernel when running in PVH mode.
>>>>> So what you suggest would end up in misleading information imo.
>>>>
>>>> Hmm, yes, I mixed "paravirtualized" with "capable to run
>>>> paravirtualized".
>>>>
>>>> So the string should depend on xen_pv_domain().
>>>
>>> But that's already expressed by pv_info.name then being "Xen PV".
>>
>> True. Okay, I'm fine with just dropping "paravirtualized".
> 
> Done. Do you want me to also make pr_info() vs printk(KERN_INFO ...)
> consistent in the function at this occasion? If so - which of the two?

pr_info(), please.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 8/9] x86/PVH: adjust function/data placement

[Jan Beulich]

Two of the variables can live in .init.data, allowing the open-coded
placing in .data to go away. Another "variable" is used to communicate a
size value only to very early assembly code, which hence can be both
const and live in .init.*. Additionally two functions were lacking
__init annotations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/platform/pvh/enlighten.c
+++ b/arch/x86/platform/pvh/enlighten.c
@@ -16,15 +16,15 @@
 /*
  * PVH variables.
  *
- * pvh_bootparams and pvh_start_info need to live in the data segment since
+ * pvh_bootparams and pvh_start_info need to live in a data segment since
  * they are used after startup_{32|64}, which clear .bss, are invoked.
  */
-struct boot_params pvh_bootparams __section(".data");
-struct hvm_start_info pvh_start_info __section(".data");
+struct boot_params __initdata pvh_bootparams;
+struct hvm_start_info __initdata pvh_start_info;
 
-unsigned int pvh_start_info_sz = sizeof(pvh_start_info);
+const unsigned int __initconst pvh_start_info_sz = sizeof(pvh_start_info);
 
-static u64 pvh_get_root_pointer(void)
+static u64 __init pvh_get_root_pointer(void)
 {
 	return pvh_start_info.rsdp_paddr;
 }
@@ -107,7 +107,7 @@ void __init __weak xen_pvh_init(struct b
 	BUG();
 }
 
-static void hypervisor_specific_init(bool xen_guest)
+static void __init hypervisor_specific_init(bool xen_guest)
 {
 	if (xen_guest)
 		xen_pvh_init(&pvh_bootparams);

Re: [PATCH 8/9] x86/PVH: adjust function/data placement

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 467 bytes --]

On 07.09.21 12:12, Jan Beulich wrote:
> Two of the variables can live in .init.data, allowing the open-coded
> placing in .data to go away. Another "variable" is used to communicate a
> size value only to very early assembly code, which hence can be both
> const and live in .init.*. Additionally two functions were lacking
> __init annotations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 9/9] xen/x86: adjust data placement

[Jan Beulich]

Both xen_pvh and xen_start_flags get written just once aeryl during
init. Using the respective annotation then allows the open-coded placing
in .data to go away.

Additionally the former, like the latter, wants exporting, or else
xen_pvh_domain() can't be used from modules.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I have to admit that it is completely unclear to me which form of
exporting I should have used: xen_domain_type is GPL-only while
xen_start_flags is not, yet both are used in similar ways, extending to
xen_pvh.

--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -75,7 +75,7 @@ EXPORT_SYMBOL_GPL(xen_have_vector_callba
  */
 enum xen_domain_type __ro_after_init xen_domain_type = XEN_NATIVE;
 EXPORT_SYMBOL_GPL(xen_domain_type);
-uint32_t xen_start_flags __section(".data") = 0;
+uint32_t __ro_after_init xen_start_flags;
 EXPORT_SYMBOL(xen_start_flags);
 
 /*
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/acpi.h>
+#include <linux/export.h>
 
 #include <xen/hvc-console.h>
 
@@ -18,10 +19,11 @@
 /*
  * PVH variables.
  *
- * The variable xen_pvh needs to live in the data segment since it is used
+ * The variable xen_pvh needs to live in a data segment since it is used
  * after startup_{32|64} is invoked, which will clear the .bss segment.
  */
-bool xen_pvh __section(".data") = 0;
+bool __ro_after_init xen_pvh;
+EXPORT_SYMBOL(xen_pvh);
 
 void __init xen_pvh_init(struct boot_params *boot_params)
 {

Re: [PATCH 9/9] xen/x86: adjust data placement

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 668 bytes --]

On 07.09.21 12:13, Jan Beulich wrote:
> Both xen_pvh and xen_start_flags get written just once aeryl during
> init. Using the respective annotation then allows the open-coded placing
> in .data to go away.
> 
> Additionally the former, like the latter, wants exporting, or else
> xen_pvh_domain() can't be used from modules.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I have to admit that it is completely unclear to me which form of
> exporting I should have used: xen_domain_type is GPL-only while
> xen_start_flags is not, yet both are used in similar ways, extending to
> xen_pvh.

I'd use EXPORT_SYMBOL_GPL(xen_pvh).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Roger Pau Monné]

On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
> In order to try to debug hypervisor side breakage from XSA-378 I found
> myself urged to finally give PVH Dom0 a try. Sadly things didn't work
> quite as expected. In the course of investigating these issues I actually
> spotted one piece of PV Dom0 breakage as well, a fix for which is also
> included here.
> 
> There are two immediate remaining issues (also mentioned in affected
> patches):
> 
> 1) It is not clear to me how PCI device reporting is to work. PV Dom0
>    reports devices as they're discovered, including ones the hypervisor
>    may not have been able to discover itself (ones on segments other
>    than 0 or hotplugged ones). The respective hypercall, however, is
>    inaccessible to PVH Dom0. Depending on the answer to this, either
>    the hypervisor will need changing (to permit the call) or patch 2
>    here will need further refinement.

I would rather prefer if we could limit the hypercall usage to only
report hotplugged segments to Xen. Then Xen would have to scan the
segment when reported and add any devices found.

Such hypercall must be used before dom0 tries to access any device, as
otherwise the BARs won't be mapped in the second stage translation and
the traps for the MCFG area won't be setup either.

> 
> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
>    console) when in a non-default mode (i.e. not 80x25 text), as the
>    necessary information (in particular about VESA-bases LFB modes) is
>    not communicated. On the hypervisor side this looks like deliberate
>    behavior, but it is unclear to me what the intentions were towards
>    an alternative model. (X may be able to access the screen depending
>    on whether it has a suitable driver besides the presently unusable
>    /dev/fb<N> based one.)

I had to admit most of my boxes are headless servers, albeit I have
one NUC I can use to test gfx stuff, so I don't really use gfx output
with Xen.

As I understand such information is fetched from the BIOS and passed
into Xen, which should then hand it over to the dom0 kernel?

I guess the only way for Linux dom0 kernel to fetch that information
would be to emulate the BIOS or drop into realmode and issue the BIOS
calls?

Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
info using the PV EFI interface?

Thanks, Roger.

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Jan Beulich]

On 14.09.2021 10:32, Roger Pau Monné wrote:
> On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
>> In order to try to debug hypervisor side breakage from XSA-378 I found
>> myself urged to finally give PVH Dom0 a try. Sadly things didn't work
>> quite as expected. In the course of investigating these issues I actually
>> spotted one piece of PV Dom0 breakage as well, a fix for which is also
>> included here.
>>
>> There are two immediate remaining issues (also mentioned in affected
>> patches):
>>
>> 1) It is not clear to me how PCI device reporting is to work. PV Dom0
>>    reports devices as they're discovered, including ones the hypervisor
>>    may not have been able to discover itself (ones on segments other
>>    than 0 or hotplugged ones). The respective hypercall, however, is
>>    inaccessible to PVH Dom0. Depending on the answer to this, either
>>    the hypervisor will need changing (to permit the call) or patch 2
>>    here will need further refinement.
> 
> I would rather prefer if we could limit the hypercall usage to only
> report hotplugged segments to Xen. Then Xen would have to scan the
> segment when reported and add any devices found.
> 
> Such hypercall must be used before dom0 tries to access any device, as
> otherwise the BARs won't be mapped in the second stage translation and
> the traps for the MCFG area won't be setup either.

This might work if hotplugging would only ever be of segments, and not
of individual devices. Yet the latter is, I think, a common case (as
far as hotplugging itself is "common").

Also don't forget about SR-IOV VFs - they would typically not be there
when booting. They would materialize when the PF driver initializes
the device. This is, I think, something that can be dealt with by
intercepting writes to the SR-IOV capability. But I wonder whether
there might be other cases where devices become "visible" only while
the Dom0 kernel is already running.

>> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
>>    console) when in a non-default mode (i.e. not 80x25 text), as the
>>    necessary information (in particular about VESA-bases LFB modes) is
>>    not communicated. On the hypervisor side this looks like deliberate
>>    behavior, but it is unclear to me what the intentions were towards
>>    an alternative model. (X may be able to access the screen depending
>>    on whether it has a suitable driver besides the presently unusable
>>    /dev/fb<N> based one.)
> 
> I had to admit most of my boxes are headless servers, albeit I have
> one NUC I can use to test gfx stuff, so I don't really use gfx output
> with Xen.
> 
> As I understand such information is fetched from the BIOS and passed
> into Xen, which should then hand it over to the dom0 kernel?

That's how PV Dom0 learns of the information, yes. See
fill_console_start_info(). (I'm in the process of eliminating the
need for some of the "fetch from BIOS" in Xen right now, but that's
not going to get us as far as being able to delete that code, no
matter how much in particular Andrew would like that to happen.)

> I guess the only way for Linux dom0 kernel to fetch that information
> would be to emulate the BIOS or drop into realmode and issue the BIOS
> calls?

Native Linux gets this information passed from the boot loader, I think
(except in the EFI case, as per below).

> Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
> info using the PV EFI interface?

There it's EFI boot services functions which can be invoked before
leaving boot services (in the native case). Aiui the PVH entry point
lives logically past any EFI boot services interaction, and hence
using them is not an option (if there was EFI firmware present in Dom0
in the first place, which I consider difficult all by itself - this
can't be the physical system's firmware, but I also don't see where
virtual firmware would be taken from).

There is no PV EFI interface to obtain video information. With the
needed information getting passed via start_info, PV has no need for
such, and I would be hesitant to add a fundamentally redundant
interface for PVH. The more that the information needed isn't EFI-
specific at all.

Jan

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Roger Pau Monné]

On Tue, Sep 14, 2021 at 11:03:23AM +0200, Jan Beulich wrote:
> On 14.09.2021 10:32, Roger Pau Monné wrote:
> > On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
> >> In order to try to debug hypervisor side breakage from XSA-378 I found
> >> myself urged to finally give PVH Dom0 a try. Sadly things didn't work
> >> quite as expected. In the course of investigating these issues I actually
> >> spotted one piece of PV Dom0 breakage as well, a fix for which is also
> >> included here.
> >>
> >> There are two immediate remaining issues (also mentioned in affected
> >> patches):
> >>
> >> 1) It is not clear to me how PCI device reporting is to work. PV Dom0
> >>    reports devices as they're discovered, including ones the hypervisor
> >>    may not have been able to discover itself (ones on segments other
> >>    than 0 or hotplugged ones). The respective hypercall, however, is
> >>    inaccessible to PVH Dom0. Depending on the answer to this, either
> >>    the hypervisor will need changing (to permit the call) or patch 2
> >>    here will need further refinement.
> > 
> > I would rather prefer if we could limit the hypercall usage to only
> > report hotplugged segments to Xen. Then Xen would have to scan the
> > segment when reported and add any devices found.
> > 
> > Such hypercall must be used before dom0 tries to access any device, as
> > otherwise the BARs won't be mapped in the second stage translation and
> > the traps for the MCFG area won't be setup either.
> 
> This might work if hotplugging would only ever be of segments, and not
> of individual devices. Yet the latter is, I think, a common case (as
> far as hotplugging itself is "common").

Right, I agree to use hypercalls to report either hotplugged segments
or devices. However I would like to avoid mandating usage of the
hypercall for non-hotplug stuff, as then OSes not having hotplug
support don't really need to care about making use of those
hypercalls.

> Also don't forget about SR-IOV VFs - they would typically not be there
> when booting. They would materialize when the PF driver initializes
> the device. This is, I think, something that can be dealt with by
> intercepting writes to the SR-IOV capability.

My plan was to indeed trap SR-IOV capability accesses, see:

https://lore.kernel.org/xen-devel/20180717094830.54806-1-roger.pau@citrix.com/

I just don't have time ATM to continue this work.

> But I wonder whether
> there might be other cases where devices become "visible" only while
> the Dom0 kernel is already running.

I would consider those kind of hotplug devices, and hence would
require the use of the hypercall in order to notify Xen about them.

> >> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
> >>    console) when in a non-default mode (i.e. not 80x25 text), as the
> >>    necessary information (in particular about VESA-bases LFB modes) is
> >>    not communicated. On the hypervisor side this looks like deliberate
> >>    behavior, but it is unclear to me what the intentions were towards
> >>    an alternative model. (X may be able to access the screen depending
> >>    on whether it has a suitable driver besides the presently unusable
> >>    /dev/fb<N> based one.)
> > 
> > I had to admit most of my boxes are headless servers, albeit I have
> > one NUC I can use to test gfx stuff, so I don't really use gfx output
> > with Xen.
> > 
> > As I understand such information is fetched from the BIOS and passed
> > into Xen, which should then hand it over to the dom0 kernel?
> 
> That's how PV Dom0 learns of the information, yes. See
> fill_console_start_info(). (I'm in the process of eliminating the
> need for some of the "fetch from BIOS" in Xen right now, but that's
> not going to get us as far as being able to delete that code, no
> matter how much in particular Andrew would like that to happen.)
> 
> > I guess the only way for Linux dom0 kernel to fetch that information
> > would be to emulate the BIOS or drop into realmode and issue the BIOS
> > calls?
> 
> Native Linux gets this information passed from the boot loader, I think
> (except in the EFI case, as per below).
> 
> > Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
> > info using the PV EFI interface?
> 
> There it's EFI boot services functions which can be invoked before
> leaving boot services (in the native case). Aiui the PVH entry point
> lives logically past any EFI boot services interaction, and hence
> using them is not an option (if there was EFI firmware present in Dom0
> in the first place, which I consider difficult all by itself - this
> can't be the physical system's firmware, but I also don't see where
> virtual firmware would be taken from).
> 
> There is no PV EFI interface to obtain video information. With the
> needed information getting passed via start_info, PV has no need for
> such, and I would be hesitant to add a fundamentally redundant
> interface for PVH. The more that the information needed isn't EFI-
> specific at all.

I think our only option is to expand the HVM start info information to
convey that data from Xen into dom0.

Thanks, Roger.

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Jan Beulich]

On 14.09.2021 13:15, Roger Pau Monné wrote:
> On Tue, Sep 14, 2021 at 11:03:23AM +0200, Jan Beulich wrote:
>> On 14.09.2021 10:32, Roger Pau Monné wrote:
>>> On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
>>>> In order to try to debug hypervisor side breakage from XSA-378 I found
>>>> myself urged to finally give PVH Dom0 a try. Sadly things didn't work
>>>> quite as expected. In the course of investigating these issues I actually
>>>> spotted one piece of PV Dom0 breakage as well, a fix for which is also
>>>> included here.
>>>>
>>>> There are two immediate remaining issues (also mentioned in affected
>>>> patches):
>>>>
>>>> 1) It is not clear to me how PCI device reporting is to work. PV Dom0
>>>>    reports devices as they're discovered, including ones the hypervisor
>>>>    may not have been able to discover itself (ones on segments other
>>>>    than 0 or hotplugged ones). The respective hypercall, however, is
>>>>    inaccessible to PVH Dom0. Depending on the answer to this, either
>>>>    the hypervisor will need changing (to permit the call) or patch 2
>>>>    here will need further refinement.
>>>
>>> I would rather prefer if we could limit the hypercall usage to only
>>> report hotplugged segments to Xen. Then Xen would have to scan the
>>> segment when reported and add any devices found.
>>>
>>> Such hypercall must be used before dom0 tries to access any device, as
>>> otherwise the BARs won't be mapped in the second stage translation and
>>> the traps for the MCFG area won't be setup either.
>>
>> This might work if hotplugging would only ever be of segments, and not
>> of individual devices. Yet the latter is, I think, a common case (as
>> far as hotplugging itself is "common").
> 
> Right, I agree to use hypercalls to report either hotplugged segments
> or devices. However I would like to avoid mandating usage of the
> hypercall for non-hotplug stuff, as then OSes not having hotplug
> support don't really need to care about making use of those
> hypercalls.
> 
>> Also don't forget about SR-IOV VFs - they would typically not be there
>> when booting. They would materialize when the PF driver initializes
>> the device. This is, I think, something that can be dealt with by
>> intercepting writes to the SR-IOV capability.
> 
> My plan was to indeed trap SR-IOV capability accesses, see:
> 
> https://lore.kernel.org/xen-devel/20180717094830.54806-1-roger.pau@citrix.com/
> 
> I just don't have time ATM to continue this work.
> 
>> But I wonder whether
>> there might be other cases where devices become "visible" only while
>> the Dom0 kernel is already running.
> 
> I would consider those kind of hotplug devices, and hence would
> require the use of the hypercall in order to notify Xen about them.

So what does this mean for the one patch? Should drivers/xen/pci.c
then be built for PVH (and then have logic added to filter boot
time device discovery), or should I restrict this to be PV-only (and
PVH would get some completely different logic added later)?

>>>> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
>>>>    console) when in a non-default mode (i.e. not 80x25 text), as the
>>>>    necessary information (in particular about VESA-bases LFB modes) is
>>>>    not communicated. On the hypervisor side this looks like deliberate
>>>>    behavior, but it is unclear to me what the intentions were towards
>>>>    an alternative model. (X may be able to access the screen depending
>>>>    on whether it has a suitable driver besides the presently unusable
>>>>    /dev/fb<N> based one.)
>>>
>>> I had to admit most of my boxes are headless servers, albeit I have
>>> one NUC I can use to test gfx stuff, so I don't really use gfx output
>>> with Xen.
>>>
>>> As I understand such information is fetched from the BIOS and passed
>>> into Xen, which should then hand it over to the dom0 kernel?
>>
>> That's how PV Dom0 learns of the information, yes. See
>> fill_console_start_info(). (I'm in the process of eliminating the
>> need for some of the "fetch from BIOS" in Xen right now, but that's
>> not going to get us as far as being able to delete that code, no
>> matter how much in particular Andrew would like that to happen.)
>>
>>> I guess the only way for Linux dom0 kernel to fetch that information
>>> would be to emulate the BIOS or drop into realmode and issue the BIOS
>>> calls?
>>
>> Native Linux gets this information passed from the boot loader, I think
>> (except in the EFI case, as per below).
>>
>>> Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
>>> info using the PV EFI interface?
>>
>> There it's EFI boot services functions which can be invoked before
>> leaving boot services (in the native case). Aiui the PVH entry point
>> lives logically past any EFI boot services interaction, and hence
>> using them is not an option (if there was EFI firmware present in Dom0
>> in the first place, which I consider difficult all by itself - this
>> can't be the physical system's firmware, but I also don't see where
>> virtual firmware would be taken from).
>>
>> There is no PV EFI interface to obtain video information. With the
>> needed information getting passed via start_info, PV has no need for
>> such, and I would be hesitant to add a fundamentally redundant
>> interface for PVH. The more that the information needed isn't EFI-
>> specific at all.
> 
> I think our only option is to expand the HVM start info information to
> convey that data from Xen into dom0.

PHV doesn't use the ordinary start_info, does it?

Jan

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Roger Pau Monné]

On Tue, Sep 14, 2021 at 01:58:29PM +0200, Jan Beulich wrote:
> On 14.09.2021 13:15, Roger Pau Monné wrote:
> > On Tue, Sep 14, 2021 at 11:03:23AM +0200, Jan Beulich wrote:
> >> On 14.09.2021 10:32, Roger Pau Monné wrote:
> >>> On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
> >>>> In order to try to debug hypervisor side breakage from XSA-378 I found
> >>>> myself urged to finally give PVH Dom0 a try. Sadly things didn't work
> >>>> quite as expected. In the course of investigating these issues I actually
> >>>> spotted one piece of PV Dom0 breakage as well, a fix for which is also
> >>>> included here.
> >>>>
> >>>> There are two immediate remaining issues (also mentioned in affected
> >>>> patches):
> >>>>
> >>>> 1) It is not clear to me how PCI device reporting is to work. PV Dom0
> >>>>    reports devices as they're discovered, including ones the hypervisor
> >>>>    may not have been able to discover itself (ones on segments other
> >>>>    than 0 or hotplugged ones). The respective hypercall, however, is
> >>>>    inaccessible to PVH Dom0. Depending on the answer to this, either
> >>>>    the hypervisor will need changing (to permit the call) or patch 2
> >>>>    here will need further refinement.
> >>>
> >>> I would rather prefer if we could limit the hypercall usage to only
> >>> report hotplugged segments to Xen. Then Xen would have to scan the
> >>> segment when reported and add any devices found.
> >>>
> >>> Such hypercall must be used before dom0 tries to access any device, as
> >>> otherwise the BARs won't be mapped in the second stage translation and
> >>> the traps for the MCFG area won't be setup either.
> >>
> >> This might work if hotplugging would only ever be of segments, and not
> >> of individual devices. Yet the latter is, I think, a common case (as
> >> far as hotplugging itself is "common").
> > 
> > Right, I agree to use hypercalls to report either hotplugged segments
> > or devices. However I would like to avoid mandating usage of the
> > hypercall for non-hotplug stuff, as then OSes not having hotplug
> > support don't really need to care about making use of those
> > hypercalls.
> > 
> >> Also don't forget about SR-IOV VFs - they would typically not be there
> >> when booting. They would materialize when the PF driver initializes
> >> the device. This is, I think, something that can be dealt with by
> >> intercepting writes to the SR-IOV capability.
> > 
> > My plan was to indeed trap SR-IOV capability accesses, see:
> > 
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fxen-devel%2F20180717094830.54806-1-roger.pau%40citrix.com%2F&data=04%7C01%7Croger.pau%40citrix.com%7C35d2502d0128484e229e08d97777087f%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637672175399546062%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=sSeE%2F4wEo5%2Fplkj2yH%2B1kpHi5c15lxJxeUxx6Cbyr4s%3D&reserved=0
> > 
> > I just don't have time ATM to continue this work.
> > 
> >> But I wonder whether
> >> there might be other cases where devices become "visible" only while
> >> the Dom0 kernel is already running.
> > 
> > I would consider those kind of hotplug devices, and hence would
> > require the use of the hypercall in order to notify Xen about them.
> 
> So what does this mean for the one patch? Should drivers/xen/pci.c
> then be built for PVH (and then have logic added to filter boot
> time device discovery), or should I restrict this to be PV-only (and
> PVH would get some completely different logic added later)?

I think we can reuse the same hypercalls for PVH, and maybe the same
code in Linux. For PVH we just need to be careful to make the
hypercalls before attempting to access the BARs (or the PCI
configuration space for the device) since there won't be any traps
setup, and BARs won't be mapped on the p2m.

It might be easier for Linux to just report every device it finds to
Xen, like it's currently done for PV dom0, instead of filtering on
whether the device has been hotplugged.

> >>>> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
> >>>>    console) when in a non-default mode (i.e. not 80x25 text), as the
> >>>>    necessary information (in particular about VESA-bases LFB modes) is
> >>>>    not communicated. On the hypervisor side this looks like deliberate
> >>>>    behavior, but it is unclear to me what the intentions were towards
> >>>>    an alternative model. (X may be able to access the screen depending
> >>>>    on whether it has a suitable driver besides the presently unusable
> >>>>    /dev/fb<N> based one.)
> >>>
> >>> I had to admit most of my boxes are headless servers, albeit I have
> >>> one NUC I can use to test gfx stuff, so I don't really use gfx output
> >>> with Xen.
> >>>
> >>> As I understand such information is fetched from the BIOS and passed
> >>> into Xen, which should then hand it over to the dom0 kernel?
> >>
> >> That's how PV Dom0 learns of the information, yes. See
> >> fill_console_start_info(). (I'm in the process of eliminating the
> >> need for some of the "fetch from BIOS" in Xen right now, but that's
> >> not going to get us as far as being able to delete that code, no
> >> matter how much in particular Andrew would like that to happen.)
> >>
> >>> I guess the only way for Linux dom0 kernel to fetch that information
> >>> would be to emulate the BIOS or drop into realmode and issue the BIOS
> >>> calls?
> >>
> >> Native Linux gets this information passed from the boot loader, I think
> >> (except in the EFI case, as per below).
> >>
> >>> Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
> >>> info using the PV EFI interface?
> >>
> >> There it's EFI boot services functions which can be invoked before
> >> leaving boot services (in the native case). Aiui the PVH entry point
> >> lives logically past any EFI boot services interaction, and hence
> >> using them is not an option (if there was EFI firmware present in Dom0
> >> in the first place, which I consider difficult all by itself - this
> >> can't be the physical system's firmware, but I also don't see where
> >> virtual firmware would be taken from).
> >>
> >> There is no PV EFI interface to obtain video information. With the
> >> needed information getting passed via start_info, PV has no need for
> >> such, and I would be hesitant to add a fundamentally redundant
> >> interface for PVH. The more that the information needed isn't EFI-
> >> specific at all.
> > 
> > I think our only option is to expand the HVM start info information to
> > convey that data from Xen into dom0.
> 
> PHV doesn't use the ordinary start_info, does it?

No, it's HVM start info as described in:

xen/include/public/arch-x86/hvm/start_info.h

We have already extended it once to add a memory map, we could extend
it another time to add the video information.

Roger.

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Jan Beulich]

On 14.09.2021 14:41, Roger Pau Monné wrote:
> On Tue, Sep 14, 2021 at 01:58:29PM +0200, Jan Beulich wrote:
>> On 14.09.2021 13:15, Roger Pau Monné wrote:
>>> On Tue, Sep 14, 2021 at 11:03:23AM +0200, Jan Beulich wrote:
>>>> On 14.09.2021 10:32, Roger Pau Monné wrote:
>>>>> On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
>>>>>> In order to try to debug hypervisor side breakage from XSA-378 I found
>>>>>> myself urged to finally give PVH Dom0 a try. Sadly things didn't work
>>>>>> quite as expected. In the course of investigating these issues I actually
>>>>>> spotted one piece of PV Dom0 breakage as well, a fix for which is also
>>>>>> included here.
>>>>>>
>>>>>> There are two immediate remaining issues (also mentioned in affected
>>>>>> patches):
>>>>>>
>>>>>> 1) It is not clear to me how PCI device reporting is to work. PV Dom0
>>>>>>    reports devices as they're discovered, including ones the hypervisor
>>>>>>    may not have been able to discover itself (ones on segments other
>>>>>>    than 0 or hotplugged ones). The respective hypercall, however, is
>>>>>>    inaccessible to PVH Dom0. Depending on the answer to this, either
>>>>>>    the hypervisor will need changing (to permit the call) or patch 2
>>>>>>    here will need further refinement.
>>>>>
>>>>> I would rather prefer if we could limit the hypercall usage to only
>>>>> report hotplugged segments to Xen. Then Xen would have to scan the
>>>>> segment when reported and add any devices found.
>>>>>
>>>>> Such hypercall must be used before dom0 tries to access any device, as
>>>>> otherwise the BARs won't be mapped in the second stage translation and
>>>>> the traps for the MCFG area won't be setup either.
>>>>
>>>> This might work if hotplugging would only ever be of segments, and not
>>>> of individual devices. Yet the latter is, I think, a common case (as
>>>> far as hotplugging itself is "common").
>>>
>>> Right, I agree to use hypercalls to report either hotplugged segments
>>> or devices. However I would like to avoid mandating usage of the
>>> hypercall for non-hotplug stuff, as then OSes not having hotplug
>>> support don't really need to care about making use of those
>>> hypercalls.
>>>
>>>> Also don't forget about SR-IOV VFs - they would typically not be there
>>>> when booting. They would materialize when the PF driver initializes
>>>> the device. This is, I think, something that can be dealt with by
>>>> intercepting writes to the SR-IOV capability.
>>>
>>> My plan was to indeed trap SR-IOV capability accesses, see:
>>>
>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fxen-devel%2F20180717094830.54806-1-roger.pau%40citrix.com%2F&data=04%7C01%7Croger.pau%40citrix.com%7C35d2502d0128484e229e08d97777087f%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637672175399546062%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=sSeE%2F4wEo5%2Fplkj2yH%2B1kpHi5c15lxJxeUxx6Cbyr4s%3D&reserved=0
>>>
>>> I just don't have time ATM to continue this work.
>>>
>>>> But I wonder whether
>>>> there might be other cases where devices become "visible" only while
>>>> the Dom0 kernel is already running.
>>>
>>> I would consider those kind of hotplug devices, and hence would
>>> require the use of the hypercall in order to notify Xen about them.
>>
>> So what does this mean for the one patch? Should drivers/xen/pci.c
>> then be built for PVH (and then have logic added to filter boot
>> time device discovery), or should I restrict this to be PV-only (and
>> PVH would get some completely different logic added later)?
> 
> I think we can reuse the same hypercalls for PVH, and maybe the same
> code in Linux. For PVH we just need to be careful to make the
> hypercalls before attempting to access the BARs (or the PCI
> configuration space for the device) since there won't be any traps
> setup, and BARs won't be mapped on the p2m.
> 
> It might be easier for Linux to just report every device it finds to
> Xen, like it's currently done for PV dom0, instead of filtering on
> whether the device has been hotplugged.

Okay. I'll leave the Linux patch as is then and instead make a Xen
patch to actually let through the necessary function(s) in
hvm_physdev_op().

>>>>>> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
>>>>>>    console) when in a non-default mode (i.e. not 80x25 text), as the
>>>>>>    necessary information (in particular about VESA-bases LFB modes) is
>>>>>>    not communicated. On the hypervisor side this looks like deliberate
>>>>>>    behavior, but it is unclear to me what the intentions were towards
>>>>>>    an alternative model. (X may be able to access the screen depending
>>>>>>    on whether it has a suitable driver besides the presently unusable
>>>>>>    /dev/fb<N> based one.)
>>>>>
>>>>> I had to admit most of my boxes are headless servers, albeit I have
>>>>> one NUC I can use to test gfx stuff, so I don't really use gfx output
>>>>> with Xen.
>>>>>
>>>>> As I understand such information is fetched from the BIOS and passed
>>>>> into Xen, which should then hand it over to the dom0 kernel?
>>>>
>>>> That's how PV Dom0 learns of the information, yes. See
>>>> fill_console_start_info(). (I'm in the process of eliminating the
>>>> need for some of the "fetch from BIOS" in Xen right now, but that's
>>>> not going to get us as far as being able to delete that code, no
>>>> matter how much in particular Andrew would like that to happen.)
>>>>
>>>>> I guess the only way for Linux dom0 kernel to fetch that information
>>>>> would be to emulate the BIOS or drop into realmode and issue the BIOS
>>>>> calls?
>>>>
>>>> Native Linux gets this information passed from the boot loader, I think
>>>> (except in the EFI case, as per below).
>>>>
>>>>> Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
>>>>> info using the PV EFI interface?
>>>>
>>>> There it's EFI boot services functions which can be invoked before
>>>> leaving boot services (in the native case). Aiui the PVH entry point
>>>> lives logically past any EFI boot services interaction, and hence
>>>> using them is not an option (if there was EFI firmware present in Dom0
>>>> in the first place, which I consider difficult all by itself - this
>>>> can't be the physical system's firmware, but I also don't see where
>>>> virtual firmware would be taken from).
>>>>
>>>> There is no PV EFI interface to obtain video information. With the
>>>> needed information getting passed via start_info, PV has no need for
>>>> such, and I would be hesitant to add a fundamentally redundant
>>>> interface for PVH. The more that the information needed isn't EFI-
>>>> specific at all.
>>>
>>> I think our only option is to expand the HVM start info information to
>>> convey that data from Xen into dom0.
>>
>> PHV doesn't use the ordinary start_info, does it?
> 
> No, it's HVM start info as described in:
> 
> xen/include/public/arch-x86/hvm/start_info.h
> 
> We have already extended it once to add a memory map, we could extend
> it another time to add the video information.

Okay, I'll try to make a(nother) patch along these lines. Since there's
a DomU counterpart in PV's start_info - where does that information get
passed for PVH? (I'm mainly wondering whether there's another approach
to consider.)

Jan

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Roger Pau Monné]

On Tue, Sep 14, 2021 at 05:13:52PM +0200, Jan Beulich wrote:
> On 14.09.2021 14:41, Roger Pau Monné wrote:
> > On Tue, Sep 14, 2021 at 01:58:29PM +0200, Jan Beulich wrote:
> >> On 14.09.2021 13:15, Roger Pau Monné wrote:
> >>> On Tue, Sep 14, 2021 at 11:03:23AM +0200, Jan Beulich wrote:
> >>>> On 14.09.2021 10:32, Roger Pau Monné wrote:
> >>>>> On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
> >>>>>> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
> >>>>>>    console) when in a non-default mode (i.e. not 80x25 text), as the
> >>>>>>    necessary information (in particular about VESA-bases LFB modes) is
> >>>>>>    not communicated. On the hypervisor side this looks like deliberate
> >>>>>>    behavior, but it is unclear to me what the intentions were towards
> >>>>>>    an alternative model. (X may be able to access the screen depending
> >>>>>>    on whether it has a suitable driver besides the presently unusable
> >>>>>>    /dev/fb<N> based one.)
> >>>>>
> >>>>> I had to admit most of my boxes are headless servers, albeit I have
> >>>>> one NUC I can use to test gfx stuff, so I don't really use gfx output
> >>>>> with Xen.
> >>>>>
> >>>>> As I understand such information is fetched from the BIOS and passed
> >>>>> into Xen, which should then hand it over to the dom0 kernel?
> >>>>
> >>>> That's how PV Dom0 learns of the information, yes. See
> >>>> fill_console_start_info(). (I'm in the process of eliminating the
> >>>> need for some of the "fetch from BIOS" in Xen right now, but that's
> >>>> not going to get us as far as being able to delete that code, no
> >>>> matter how much in particular Andrew would like that to happen.)
> >>>>
> >>>>> I guess the only way for Linux dom0 kernel to fetch that information
> >>>>> would be to emulate the BIOS or drop into realmode and issue the BIOS
> >>>>> calls?
> >>>>
> >>>> Native Linux gets this information passed from the boot loader, I think
> >>>> (except in the EFI case, as per below).
> >>>>
> >>>>> Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
> >>>>> info using the PV EFI interface?
> >>>>
> >>>> There it's EFI boot services functions which can be invoked before
> >>>> leaving boot services (in the native case). Aiui the PVH entry point
> >>>> lives logically past any EFI boot services interaction, and hence
> >>>> using them is not an option (if there was EFI firmware present in Dom0
> >>>> in the first place, which I consider difficult all by itself - this
> >>>> can't be the physical system's firmware, but I also don't see where
> >>>> virtual firmware would be taken from).
> >>>>
> >>>> There is no PV EFI interface to obtain video information. With the
> >>>> needed information getting passed via start_info, PV has no need for
> >>>> such, and I would be hesitant to add a fundamentally redundant
> >>>> interface for PVH. The more that the information needed isn't EFI-
> >>>> specific at all.
> >>>
> >>> I think our only option is to expand the HVM start info information to
> >>> convey that data from Xen into dom0.
> >>
> >> PHV doesn't use the ordinary start_info, does it?
> > 
> > No, it's HVM start info as described in:
> > 
> > xen/include/public/arch-x86/hvm/start_info.h
> > 
> > We have already extended it once to add a memory map, we could extend
> > it another time to add the video information.
> 
> Okay, I'll try to make a(nother) patch along these lines. Since there's
> a DomU counterpart in PV's start_info - where does that information get
> passed for PVH? (I'm mainly wondering whether there's another approach
> to consider.)

We don't pass the video information at all for PVH, neither in domU or
dom0 modes if that's what you mean. Not sure what video information we
could pass for domU anyway, as that would be a PV framebuffer that
would need setup ATM. Maybe we could at some point provide some kind
of emulated or passed through card.

The information contained in start_info that's needed for PVH is
passed using hvm params, just like it's done for plain HVM guests. We
could pass the video information in a hvm param I guess, but it would
require stealing guest memory to store it (and mark as reserved in
the memory map). Not sure that's better than expanding HVM start info.

Maybe there's another hypercall interface I'm missing we could use to
propagate that information to dom0?

Thanks, Roger.

Re: [PATCH 0/9] xen/x86: PVH Dom0 fixes and fallout adjustments

[Jan Beulich]

On 14.09.2021 18:27, Roger Pau Monné wrote:
> On Tue, Sep 14, 2021 at 05:13:52PM +0200, Jan Beulich wrote:
>> On 14.09.2021 14:41, Roger Pau Monné wrote:
>>> On Tue, Sep 14, 2021 at 01:58:29PM +0200, Jan Beulich wrote:
>>>> On 14.09.2021 13:15, Roger Pau Monné wrote:
>>>>> On Tue, Sep 14, 2021 at 11:03:23AM +0200, Jan Beulich wrote:
>>>>>> On 14.09.2021 10:32, Roger Pau Monné wrote:
>>>>>>> On Tue, Sep 07, 2021 at 12:04:34PM +0200, Jan Beulich wrote:
>>>>>>>> 2) Dom0, unlike in the PV case, cannot access the screen (to use as a
>>>>>>>>    console) when in a non-default mode (i.e. not 80x25 text), as the
>>>>>>>>    necessary information (in particular about VESA-bases LFB modes) is
>>>>>>>>    not communicated. On the hypervisor side this looks like deliberate
>>>>>>>>    behavior, but it is unclear to me what the intentions were towards
>>>>>>>>    an alternative model. (X may be able to access the screen depending
>>>>>>>>    on whether it has a suitable driver besides the presently unusable
>>>>>>>>    /dev/fb<N> based one.)
>>>>>>>
>>>>>>> I had to admit most of my boxes are headless servers, albeit I have
>>>>>>> one NUC I can use to test gfx stuff, so I don't really use gfx output
>>>>>>> with Xen.
>>>>>>>
>>>>>>> As I understand such information is fetched from the BIOS and passed
>>>>>>> into Xen, which should then hand it over to the dom0 kernel?
>>>>>>
>>>>>> That's how PV Dom0 learns of the information, yes. See
>>>>>> fill_console_start_info(). (I'm in the process of eliminating the
>>>>>> need for some of the "fetch from BIOS" in Xen right now, but that's
>>>>>> not going to get us as far as being able to delete that code, no
>>>>>> matter how much in particular Andrew would like that to happen.)
>>>>>>
>>>>>>> I guess the only way for Linux dom0 kernel to fetch that information
>>>>>>> would be to emulate the BIOS or drop into realmode and issue the BIOS
>>>>>>> calls?
>>>>>>
>>>>>> Native Linux gets this information passed from the boot loader, I think
>>>>>> (except in the EFI case, as per below).
>>>>>>
>>>>>>> Is that an issue on UEFI also, or there dom0 can fetch the framebuffer
>>>>>>> info using the PV EFI interface?
>>>>>>
>>>>>> There it's EFI boot services functions which can be invoked before
>>>>>> leaving boot services (in the native case). Aiui the PVH entry point
>>>>>> lives logically past any EFI boot services interaction, and hence
>>>>>> using them is not an option (if there was EFI firmware present in Dom0
>>>>>> in the first place, which I consider difficult all by itself - this
>>>>>> can't be the physical system's firmware, but I also don't see where
>>>>>> virtual firmware would be taken from).
>>>>>>
>>>>>> There is no PV EFI interface to obtain video information. With the
>>>>>> needed information getting passed via start_info, PV has no need for
>>>>>> such, and I would be hesitant to add a fundamentally redundant
>>>>>> interface for PVH. The more that the information needed isn't EFI-
>>>>>> specific at all.
>>>>>
>>>>> I think our only option is to expand the HVM start info information to
>>>>> convey that data from Xen into dom0.
>>>>
>>>> PHV doesn't use the ordinary start_info, does it?
>>>
>>> No, it's HVM start info as described in:
>>>
>>> xen/include/public/arch-x86/hvm/start_info.h
>>>
>>> We have already extended it once to add a memory map, we could extend
>>> it another time to add the video information.
>>
>> Okay, I'll try to make a(nother) patch along these lines. Since there's
>> a DomU counterpart in PV's start_info - where does that information get
>> passed for PVH? (I'm mainly wondering whether there's another approach
>> to consider.)
> 
> We don't pass the video information at all for PVH, neither in domU or
> dom0 modes if that's what you mean. Not sure what video information we
> could pass for domU anyway, as that would be a PV framebuffer that
> would need setup ATM. Maybe we could at some point provide some kind
> of emulated or passed through card.
> 
> The information contained in start_info that's needed for PVH is
> passed using hvm params, just like it's done for plain HVM guests.

This is what I was referring to; I'm sorry for having been unclear.
It's no video _mode_ information, but information on hot to get at
the console.

> We
> could pass the video information in a hvm param I guess, but it would
> require stealing guest memory to store it (and mark as reserved in
> the memory map). Not sure that's better than expanding HVM start info.

I don't think it would be; a param doesn't seem a good fit here,
and I have to admit I'm not even convinced its a good fit for
xenstore and console coordinates (that's fine for HVM; the only
reason I can see for PVH to use the same is the expectation of
the line between both to become increasingly blurred).

> Maybe there's another hypercall interface I'm missing we could use to
> propagate that information to dom0?

I don't think there is; if anything we'd have to add something.

Jan