On 30/11/2017 19:23, Maran Wilson wrote:
> Are you saying the Linux PVH entry code (such as init_pvh_bootparams())
> should use the fw_cfg interface to read the e820 memory map data and put
> it into the zeropage? Basically, keeping the patch very much like it
> already is, just extracting the e820 data via the fw_cfg interface
> instead of from the second module of start_info struct?
Yes.
> If that is the case, I guess I'm a bit hesitant to throw the QEMU
> specific fw_cfg interface into the mix on the Linux PVH side when the
> existing PVH ABI already seems to contain an interface for passing
> modules/blobs to the guest. But if you feel there is a compelling reason
> to use the fw_cfg interface here, I'm happy to explore that approach
> further.
I think the same holds true for Xen, but it is still using a hypercall
to get the memory map. In the end, using fw_cfg seems closest to what
the Xen code does.
There are other possibilities:
1) defining a v2 PVH ABI that includes the e820 map would also be a
possibility.
2) modify enlighten_pvh.c to get the start info in multiboot format,
something like:
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 98ab17673454..656e41449db0 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -88,19 +88,22 @@ void __init xen_prepare_pvh(void)
u32 msr;
u64 pfn;
- if (pvh_start_info.magic != XEN_HVM_START_MAGIC_VALUE) {
+ if (pvh_start_info.magic == XEN_HVM_START_MAGIC_VALUE) {
+ xen_pvh = 1;
+
+ init_pvh_bootparams_xen();
+
+ msr = cpuid_ebx(xen_cpuid_base() + 2);
+ pfn = __pa(hypercall_page);
+ wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
+
+ x86_init.oem.arch_setup = xen_pvh_arch_setup;
+ } else if (pvh_start_info.magic == MULTIBOOT_INFO_MAGIC_VALUE) {
+ init_pvh_bootparams_multiboot();
+
+ } else {
xen_raw_printk("Error: Unexpected magic value (0x%08x)\n",
pvh_start_info.magic);
BUG();
}
-
- xen_pvh = 1;
-
- msr = cpuid_ebx(xen_cpuid_base() + 2);
- pfn = __pa(hypercall_page);
- wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
-
- init_pvh_bootparams();
-
- x86_init.oem.arch_setup = xen_pvh_arch_setup;
}
Note that this would *not* be a multiboot-format kernel, as it would
still have the Xen PVH ELF note. It would just reuse the format of
the start info struct.
However, I think it is simpler to just use the e820 memory map from
fw_cfg.
Paolo
Just FYI: I sent out a v2 of this patch but in doing so I moved a few
people from the "to" line to the "cc" line.
For anyone who previously did not comment but still wanted to follow the
discussion, here's the link to the v2 email:
https://lkml.org/lkml/2017/12/7/1624
Thanks,
-Maran
On 12/1/2017 12:08 AM, Paolo Bonzini wrote:
> On 30/11/2017 19:23, Maran Wilson wrote:
>> Are you saying the Linux PVH entry code (such as init_pvh_bootparams())
>> should use the fw_cfg interface to read the e820 memory map data and put
>> it into the zeropage? Basically, keeping the patch very much like it
>> already is, just extracting the e820 data via the fw_cfg interface
>> instead of from the second module of start_info struct?
> Yes.
>
>> If that is the case, I guess I'm a bit hesitant to throw the QEMU
>> specific fw_cfg interface into the mix on the Linux PVH side when the
>> existing PVH ABI already seems to contain an interface for passing
>> modules/blobs to the guest. But if you feel there is a compelling reason
>> to use the fw_cfg interface here, I'm happy to explore that approach
>> further.
> I think the same holds true for Xen, but it is still using a hypercall
> to get the memory map. In the end, using fw_cfg seems closest to what
> the Xen code does.
>
> There are other possibilities:
>
> 1) defining a v2 PVH ABI that includes the e820 map would also be a
> possibility.
>
> 2) modify enlighten_pvh.c to get the start info in multiboot format,
> something like:
>
> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
> index 98ab17673454..656e41449db0 100644
> --- a/arch/x86/xen/enlighten_pvh.c
> +++ b/arch/x86/xen/enlighten_pvh.c
> @@ -88,19 +88,22 @@ void __init xen_prepare_pvh(void)
> u32 msr;
> u64 pfn;
>
> - if (pvh_start_info.magic != XEN_HVM_START_MAGIC_VALUE) {
> + if (pvh_start_info.magic == XEN_HVM_START_MAGIC_VALUE) {
> + xen_pvh = 1;
> +
> + init_pvh_bootparams_xen();
> +
> + msr = cpuid_ebx(xen_cpuid_base() + 2);
> + pfn = __pa(hypercall_page);
> + wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
> +
> + x86_init.oem.arch_setup = xen_pvh_arch_setup;
> + } else if (pvh_start_info.magic == MULTIBOOT_INFO_MAGIC_VALUE) {
> + init_pvh_bootparams_multiboot();
> +
> + } else {
> xen_raw_printk("Error: Unexpected magic value (0x%08x)\n",
> pvh_start_info.magic);
> BUG();
> }
> -
> - xen_pvh = 1;
> -
> - msr = cpuid_ebx(xen_cpuid_base() + 2);
> - pfn = __pa(hypercall_page);
> - wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
> -
> - init_pvh_bootparams();
> -
> - x86_init.oem.arch_setup = xen_pvh_arch_setup;
> }
>
>
> Note that this would *not* be a multiboot-format kernel, as it would
> still have the Xen PVH ELF note. It would just reuse the format of
> the start info struct.
>
> However, I think it is simpler to just use the e820 memory map from
> fw_cfg.
>
> Paolo