Xen devel - Oct 2012 - [PATCH] tools/hvmloader: move shared_info to reserved memory area

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1351101387 -7200
# Node ID 6a0c73ae9ce5cca72f788c0e0f8fd6872010d83e
# Parent  22e08c9ac770db07c3c3e7c844aa7153050939f3
tools/hvmloader: move shared_info to reserved memory area

Reserve a range of 1MB for the HVM shared info page at 0xFE700000. This
area is already marked as reserved in the E820 map. The purpose of this
change is to provide Linux PVonHVM guests with a fixed page outside of
ordinary RAM. If the shared page is located in RAM it would be
overwritten during a kexec boot.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 22e08c9ac770 -r 6a0c73ae9ce5 tools/firmware/hvmloader/config.h
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -68,6 +68,8 @@ extern unsigned long pci_mem_start, pci_
 /* NB. ACPI_INFO_PHYSICAL_ADDRESS *MUST* match definition in acpi/dsdt.asl! */
 #define ACPI_INFO_PHYSICAL_ADDRESS    0xFC000000
 #define RESERVED_MEMORY_DYNAMIC       0xFC001000
+#define HVM_SHARED_INFO_AREA          0xFE700000
+#define HVM_SHARED_INFO_SIZE          0x00100000
 
 extern unsigned long scratch_start;
 
diff -r 22e08c9ac770 -r 6a0c73ae9ce5 tools/firmware/hvmloader/util.c
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -433,11 +433,18 @@ void *mem_alloc(uint32_t size, uint32_t 
     if ( align < 16 )
         align = 16;
 
+retry:
     s = (reserve + align) & ~(align - 1);
     e = s + size - 1;
 
     BUG_ON((e < s) || (e >> PAGE_SHIFT) >=
hvm_info->reserved_mem_pgstart);
 
+    /* Skip the shared info region */
+    if (s <= HVM_SHARED_INFO_AREA && e >= HVM_SHARED_INFO_AREA) {
+	    reserve = HVM_SHARED_INFO_AREA + HVM_SHARED_INFO_SIZE - 1;
+	    goto retry;
+    }
+
     while ( (reserve >> PAGE_SHIFT) != (e >> PAGE_SHIFT) )
     {
         reserve += PAGE_SIZE;
@@ -765,7 +772,7 @@ struct shared_info *get_shared_info(void
     xatp.domid = DOMID_SELF;
     xatp.space = XENMAPSPACE_shared_info;
     xatp.idx   = 0;
-    xatp.gpfn  = mem_hole_alloc(1);
+    xatp.gpfn  = HVM_SHARED_INFO_AREA >> PAGE_SHIFT;
     shared_info = (struct shared_info *)(xatp.gpfn << PAGE_SHIFT);
     if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
         BUG();

On 24/10/2012 10:57, "Olaf Hering" <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1351101387 -7200
> # Node ID 6a0c73ae9ce5cca72f788c0e0f8fd6872010d83e
> # Parent  22e08c9ac770db07c3c3e7c844aa7153050939f3
> tools/hvmloader: move shared_info to reserved memory area
> 
> Reserve a range of 1MB for the HVM shared info page at 0xFE700000. This
> area is already marked as reserved in the E820 map. The purpose of this
> change is to provide Linux PVonHVM guests with a fixed page outside of
> ordinary RAM. If the shared page is located in RAM it would be
> overwritten during a kexec boot.
I don''t think hvmloader actually needs to map shared-info to the new
location, it justs needs to guarantee that this location is unused, and
document it so that it never *becomes* used in future.

Which can be as simple as the attached patch (in fact all the changes apart
from introducing GUEST_RESERVED_{START,END} are really cleaning up and
bug-fixing the out-of-space checks in the mem_hole_alloc/mem_alloc
functions).

This then just requires that the guest maps shared-info to FE700000 itself.
Should be quite easy. :)

 -- Keir
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff -r 22e08c9ac770 -r 6a0c73ae9ce5 tools/firmware/hvmloader/config.h
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -68,6 +68,8 @@ extern unsigned long pci_mem_start, pci_
>  /* NB. ACPI_INFO_PHYSICAL_ADDRESS *MUST* match definition in
acpi/dsdt.asl!
> */
>  #define ACPI_INFO_PHYSICAL_ADDRESS    0xFC000000
>  #define RESERVED_MEMORY_DYNAMIC       0xFC001000
> +#define HVM_SHARED_INFO_AREA          0xFE700000
> +#define HVM_SHARED_INFO_SIZE          0x00100000
>  
>  extern unsigned long scratch_start;
>  
> diff -r 22e08c9ac770 -r 6a0c73ae9ce5 tools/firmware/hvmloader/util.c
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -433,11 +433,18 @@ void *mem_alloc(uint32_t size, uint32_t
>      if ( align < 16 )
>          align = 16;
>  
> +retry:
>      s = (reserve + align) & ~(align - 1);
>      e = s + size - 1;
>  
>      BUG_ON((e < s) || (e >> PAGE_SHIFT) >=
hvm_info->reserved_mem_pgstart);
>  
> +    /* Skip the shared info region */
> +    if (s <= HVM_SHARED_INFO_AREA && e >=
HVM_SHARED_INFO_AREA) {
> +     reserve = HVM_SHARED_INFO_AREA + HVM_SHARED_INFO_SIZE - 1;
> +     goto retry;
> +    }
> +
>      while ( (reserve >> PAGE_SHIFT) != (e >> PAGE_SHIFT) )
>      {
>          reserve += PAGE_SIZE;
> @@ -765,7 +772,7 @@ struct shared_info *get_shared_info(void
>      xatp.domid = DOMID_SELF;
>      xatp.space = XENMAPSPACE_shared_info;
>      xatp.idx   = 0;
> -    xatp.gpfn  = mem_hole_alloc(1);
> +    xatp.gpfn  = HVM_SHARED_INFO_AREA >> PAGE_SHIFT;
>      shared_info = (struct shared_info *)(xatp.gpfn << PAGE_SHIFT);
>      if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>          BUG();
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Wed, Oct 24, Keir Fraser wrote:
> Which can be as simple as the attached patch (in fact all the changes apart
> from introducing GUEST_RESERVED_{START,END} are really cleaning up and
> bug-fixing the out-of-space checks in the mem_hole_alloc/mem_alloc
> functions).
> 
> This then just requires that the guest maps shared-info to FE700000 itself.
> Should be quite easy. :)
The patch works for me. And the kernel patch I sent yesterday works as
well.
Is the memory area starting from 0xFC000000 also reserved in older
versions, such as Xen3?

Olaf

On Thu, Oct 25, Olaf Hering wrote:
> On Wed, Oct 24, Keir Fraser wrote:
> 
> > Which can be as simple as the attached patch (in fact all the changes
apart
> > from introducing GUEST_RESERVED_{START,END} are really cleaning up and
> > bug-fixing the out-of-space checks in the mem_hole_alloc/mem_alloc
> > functions).
> > 
> > This then just requires that the guest maps shared-info to FE700000
itself.
> > Should be quite easy. :)
> 
> The patch works for me. And the kernel patch I sent yesterday works as
> well.
> Is the memory area starting from 0xFC000000 also reserved in older
> versions, such as Xen3?
And if the guest runs on an older tool stack, is there a slim chance
that something allocated memory up to 0xFE700000?

Olaf

On 25/10/2012 00:51, "Olaf Hering" <olaf@aepfle.de> wrote:
> On Thu, Oct 25, Olaf Hering wrote:
> 
>> On Wed, Oct 24, Keir Fraser wrote:
>> 
>>> Which can be as simple as the attached patch (in fact all the
changes apart
>>> from introducing GUEST_RESERVED_{START,END} are really cleaning up
and
>>> bug-fixing the out-of-space checks in the mem_hole_alloc/mem_alloc
>>> functions).
>>> 
>>> This then just requires that the guest maps shared-info to FE700000
itself.
>>> Should be quite easy. :)
>> 
>> The patch works for me. And the kernel patch I sent yesterday works as
>> well.
>> Is the memory area starting from 0xFC000000 also reserved in older
>> versions, such as Xen3?
It is marked as E820_RESERVED in the e820 map as far back as Xen-3.4.0
(released Spring 2009). Before that it was not covered by an e820 entry, and
there is a slim chance your guest kernel may decide to map something else at
FE700000 (PCI BAR remapping f.ex)?
> And if the guest runs on an older tool stack, is there a slim chance
> that something allocated memory up to 0xFE700000?
Again, back as far as at least Xen-3.4.0, nothing would ever have got mapped
at FE700000. Earlier than that, can''t be as authoritative, but I think
it''s
very unlikely.

 -- Keir
> Olaf

On 25/10/2012 04:33, "Keir Fraser" <keir.xen@gmail.com> wrote:
> On 25/10/2012 00:51, "Olaf Hering" <olaf@aepfle.de> wrote:
> 
>> On Thu, Oct 25, Olaf Hering wrote:
>> 
>>> On Wed, Oct 24, Keir Fraser wrote:
>>> 
>>>> Which can be as simple as the attached patch (in fact all the
changes apart
>>>> from introducing GUEST_RESERVED_{START,END} are really cleaning
up and
>>>> bug-fixing the out-of-space checks in the
mem_hole_alloc/mem_alloc
>>>> functions).
>>>> 
>>>> This then just requires that the guest maps shared-info to
FE700000 itself.
>>>> Should be quite easy. :)
>>> 
>>> The patch works for me. And the kernel patch I sent yesterday works
as
>>> well.
>>> Is the memory area starting from 0xFC000000 also reserved in older
>>> versions, such as Xen3?
> 
> It is marked as E820_RESERVED in the e820 map as far back as Xen-3.4.0
> (released Spring 2009). Before that it was not covered by an e820 entry,
and
> there is a slim chance your guest kernel may decide to map something else
at
> FE700000 (PCI BAR remapping f.ex)?
> 
>> And if the guest runs on an older tool stack, is there a slim chance
>> that something allocated memory up to 0xFE700000?
> 
> Again, back as far as at least Xen-3.4.0, nothing would ever have got
mapped
> at FE700000. Earlier than that, can''t be as authoritative, but I
think it''s
> very unlikely.
To be honest, given that the XenPVHVM extensions to Linux won''t have
been
tested on such old hypervisors, it wouldn''t be a bad thing to bail on
setting up the extensions when you detect running on a really old Xen
version (e.g., earlier than 3.4.0) anyway. There''s a fair chance of
doing
more harm than good?

 -- Keir
>  -- Keir
> 
>> Olaf
> 
>

On Thu, Oct 25, Keir Fraser wrote:
> To be honest, given that the XenPVHVM extensions to Linux won''t
have been
> tested on such old hypervisors, it wouldn''t be a bad thing to bail
on
> setting up the extensions when you detect running on a really old Xen
> version (e.g., earlier than 3.4.0) anyway. There''s a fair chance
of doing
> more harm than good?
I could stick such a check into
arch/x86/xen/enlighten.c:x86_hyper_xen_hvm->detect, by rearranging the
code of xen_hvm_platform and init_hvm_pv_info.  Konrad, what do you
think about that? Recent changes indicate that you did some testing on
3.4 based hosts.

Olaf

On Thu, 2012-10-25 at 12:46 +0100, Olaf Hering wrote:
> On Thu, Oct 25, Keir Fraser wrote:
> 
> > To be honest, given that the XenPVHVM extensions to Linux
won''t have been
> > tested on such old hypervisors, it wouldn''t be a bad thing to
bail on
> > setting up the extensions when you detect running on a really old Xen
> > version (e.g., earlier than 3.4.0) anyway. There''s a fair
chance of doing
> > more harm than good?
> 
> I could stick such a check into
> arch/x86/xen/enlighten.c:x86_hyper_xen_hvm->detect, by rearranging the
> code of xen_hvm_platform and init_hvm_pv_info.  Konrad, what do you
> think about that?
If you do something like this please could you add a command line option
to allow it to be forced both off and on?

I suppose this relates a bit to xen_emul_unplug too?
> Recent changes indicate that you did some testing on
> 3.4 based hosts.
NB Keir said "earlier than 3.4". I think we would want to try and
support 3.4, but not 3.3.

Ian.

On Thu, Oct 25, Ian Campbell wrote:
> NB Keir said "earlier than 3.4". I think we would want to try and
> support 3.4, but not 3.3.
Absolutely.

Olaf

On Thu, Oct 25, 2012 at 01:46:45PM +0200, Olaf Hering
wrote:
> On Thu, Oct 25, Keir Fraser wrote:
> 
> > To be honest, given that the XenPVHVM extensions to Linux
won''t have been
> > tested on such old hypervisors, it wouldn''t be a bad thing to
bail on
> > setting up the extensions when you detect running on a really old Xen
> > version (e.g., earlier than 3.4.0) anyway. There''s a fair
chance of doing
> > more harm than good?
> 
> I could stick such a check into
> arch/x86/xen/enlighten.c:x86_hyper_xen_hvm->detect, by rearranging the
> code of xen_hvm_platform and init_hvm_pv_info.  Konrad, what do you
> think about that? Recent changes indicate that you did some testing on
> 3.4 based hosts.
Sure. It might make sense to streamline this a bit. Meaning after the
version check, have an int (or function) called
''xen_old_hypervisor()''
which your code and the XenBus code could call?

That way on ARM that function can just become a nop while on X86 we
do the cpuids (or if those had already been done - we just return
true or false).

On Fri, Oct 26, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 25, 2012 at 01:46:45PM +0200, Olaf Hering wrote:
> > On Thu, Oct 25, Keir Fraser wrote:
> > 
> > > To be honest, given that the XenPVHVM extensions to Linux
won''t have been
> > > tested on such old hypervisors, it wouldn''t be a bad
thing to bail on
> > > setting up the extensions when you detect running on a really old
Xen
> > > version (e.g., earlier than 3.4.0) anyway. There''s a
fair chance of doing
> > > more harm than good?
> > 
> > I could stick such a check into
> > arch/x86/xen/enlighten.c:x86_hyper_xen_hvm->detect, by rearranging
the
> > code of xen_hvm_platform and init_hvm_pv_info.  Konrad, what do you
> > think about that? Recent changes indicate that you did some testing on
> > 3.4 based hosts.
> 
> Sure. It might make sense to streamline this a bit. Meaning after the
> version check, have an int (or function) called
''xen_old_hypervisor()''
> which your code and the XenBus code could call?
Oh, my take would be to return false in xen_hvm_platform if the
hypervisor is 3.3 or older, so that the guest does not use the PVonHVM
extensions.

Olaf

On Fri, Oct 26, Olaf Hering wrote:
> Oh, my take would be to return false in xen_hvm_platform if the
> hypervisor is 3.3 or older, so that the guest does not use the PVonHVM
> extensions.
Like this untested patch:

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 0cc41f8..8566fa8 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1543,17 +1543,10 @@ static void __init xen_hvm_init_shared_info(void)
 
 static void __init init_hvm_pv_info(void)
 {
-	int major, minor;
 	uint32_t eax, ebx, ecx, edx, pages, msr, base;
 	u64 pfn;
 
 	base = xen_cpuid_base();
-	cpuid(base + 1, &eax, &ebx, &ecx, &edx);
-
-	major = eax >> 16;
-	minor = eax & 0xffff;
-	printk(KERN_INFO "Xen version %d.%d.\n", major, minor);
-
 	cpuid(base + 2, &pages, &msr, &ecx, &edx);
 
 	pfn = __pa(hypercall_page);
@@ -1604,13 +1597,29 @@ static void __init xen_hvm_guest_init(void)
 
 static bool __init xen_hvm_platform(void)
 {
+	int major, minor, old = 0;
+	uint32_t eax, ebx, ecx, edx, base;
+	bool usable = true;
+
 	if (xen_pv_domain())
 		return false;
 
-	if (!xen_cpuid_base())
+	base = xen_cpuid_base();
+	if (!base)
 		return false;
 
-	return true;
+	cpuid(base + 1, &eax, &ebx, &ecx, &edx);
+
+	major = eax >> 16;
+	minor = eax & 0xffff;
+
+	/* Require at least Xen 3.4 */
+	if (major < 3 || (major == 3 && minor < 4))
+		usable = false;
+	printk(KERN_INFO "Xen version %d.%d.%s\n",
+		major, minor, usable ? "" : " (too old)");
+
+	return usable;
 }
 
 bool xen_hvm_need_lapic(void)

On 26/10/2012 08:51, "Olaf Hering" <olaf@aepfle.de> wrote:
> On Fri, Oct 26, Olaf Hering wrote:
> 
>> Oh, my take would be to return false in xen_hvm_platform if the
>> hypervisor is 3.3 or older, so that the guest does not use the PVonHVM
>> extensions.
> 
> Like this untested patch:
Looks okay to me.

 -- Keir
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 0cc41f8..8566fa8 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1543,17 +1543,10 @@ static void __init xen_hvm_init_shared_info(void)
>  
>  static void __init init_hvm_pv_info(void)
>  {
> - int major, minor;
> uint32_t eax, ebx, ecx, edx, pages, msr, base;
> u64 pfn;
>  
> base = xen_cpuid_base();
> - cpuid(base + 1, &eax, &ebx, &ecx, &edx);
> -
> - major = eax >> 16;
> - minor = eax & 0xffff;
> - printk(KERN_INFO "Xen version %d.%d.\n", major, minor);
> -
> cpuid(base + 2, &pages, &msr, &ecx, &edx);
>  
> pfn = __pa(hypercall_page);
> @@ -1604,13 +1597,29 @@ static void __init xen_hvm_guest_init(void)
>  
>  static bool __init xen_hvm_platform(void)
>  {
> + int major, minor, old = 0;
> + uint32_t eax, ebx, ecx, edx, base;
> + bool usable = true;
> +
> if (xen_pv_domain())
> return false;
>  
> - if (!xen_cpuid_base())
> + base = xen_cpuid_base();
> + if (!base)
> return false;
>  
> - return true;
> + cpuid(base + 1, &eax, &ebx, &ecx, &edx);
> +
> + major = eax >> 16;
> + minor = eax & 0xffff;
> +
> + /* Require at least Xen 3.4 */
> + if (major < 3 || (major == 3 && minor < 4))
> +  usable = false;
> + printk(KERN_INFO "Xen version %d.%d.%s\n",
> +  major, minor, usable ? "" : " (too old)");
> +
> + return usable;
>  }
>  
>  bool xen_hvm_need_lapic(void)