[PATCH RFC 0/4] hvmloader: make OVMF work with Xen

[PATCH RFC 0/4] hvmloader: make OVMF work with Xen

[Wei Liu]

This is the Xen side patches.

The important bits are setting up ovmf_info and E820 map. I'm particular
interested what to be included in ovmf_info.

Wei.

Wei Liu (4):
  hvmloader/ovmf: increase blob size to 2MB
  hvmloader/ovmf: show OVMF_BEGIN as bios address
  hvmloader/ovmf: setup ovmf_info
  hvmloader:ovmf: setup E820 map

 tools/firmware/hvmloader/ovmf.c |   58 +++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

-- 
1.7.10.4

[PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB

[Wei Liu]

The debug build of OVMF now can be as large as 2MB.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/ovmf.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index ee4cbbf..5c83626 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -38,8 +38,8 @@
 #define ROM_INCLUDE_OVMF
 #include "roms.inc"
 
-#define OVMF_BEGIN              0xFFF00000ULL
-#define OVMF_SIZE               0x00100000ULL
+#define OVMF_BEGIN              0xFFE00000ULL
+#define OVMF_SIZE               0x00200000ULL
 #define OVMF_MAXOFFSET          0x000FFFFFULL
 #define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
 #define LOWCHUNK_BEGIN          0x000F0000
-- 
1.7.10.4

[PATCH RFC 2/4] hvmloader/ovmf: show OVMF_BEGIN as bios address

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/ovmf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 5c83626..7826095 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -99,7 +99,7 @@ struct bios_config ovmf_config =  {
     .image = ovmf,
     .image_size = sizeof(ovmf),
 
-    .bios_address = 0,
+    .bios_address = OVMF_BEGIN,
     .bios_load = ovmf_load,
 
     .load_roms = 0,
-- 
1.7.10.4

[PATCH RFC 3/4] hvmloader/ovmf: setup ovmf_info

[Wei Liu]

OVMF info contains E820 map allocated by hvmloader. This info is passed
to OVMF to help it do proper initialization.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/ovmf.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 7826095..c253083 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -46,10 +46,45 @@
 #define LOWCHUNK_SIZE           0x00010000
 #define LOWCHUNK_MAXOFFSET      0x0000FFFF
 #define LOWCHUNK_END            (OVMF_BEGIN + OVMF_SIZE)
+#define OVMF_INFO_PHYSICAL_ADDRESS 0X00001000
 
 extern unsigned char dsdt_anycpu[];
 extern int dsdt_anycpu_len;
 
+struct ovmf_info {
+    char signature[11]; /* XenHVMOVMF\0 */
+    uint8_t length;     /* Length of this struct */
+    uint8_t checksum;   /* Set such that the sum over bytes 0..length == 0 */
+    /*
+     * Physical address of the e820 table, contains e820_nr entries.
+     */
+    uint32_t e820;
+    uint32_t e820_nr;
+} __attribute__ ((packed));
+
+static void ovmf_setup_bios_info(void)
+{
+    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
+
+    memset(info, 0, sizeof(*info));
+
+    memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature));
+    info->length = sizeof(*info);
+}
+
+static void ovmf_finish_bios_info(void)
+{
+    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
+    uint32_t i;
+    uint8_t checksum;
+
+    checksum = 0;
+    for ( i = 0; i < info->length; i++ )
+        checksum += ((uint8_t *)(info))[i];
+
+    info->checksum = -checksum;
+}
+
 static void ovmf_load(const struct bios_config *config)
 {
     xen_pfn_t mfn;
@@ -104,8 +139,8 @@ struct bios_config ovmf_config =  {
 
     .load_roms = 0,
 
-    .bios_info_setup = NULL,
-    .bios_info_finish = NULL,
+    .bios_info_setup = ovmf_setup_bios_info,
+    .bios_info_finish = ovmf_finish_bios_info,
 
     .e820_setup = NULL,
 
-- 
1.7.10.4

[PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Wei Liu]

E820 map will be used by OVMF to create memory map.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index c253083..52ccd0d 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
         SMBIOS_PHYSICAL_END);
 }
 
+static void ovmf_setup_e820(void)
+{
+    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
+    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
+    info->e820 = (uint32_t)e820;
+
+    /* OVMF doesn't load bios image below 0x100000 */
+    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
+    dump_e820_table(e820, info->e820_nr);
+}
+
 struct bios_config ovmf_config =  {
     .name = "OVMF",
 
@@ -142,7 +153,7 @@ struct bios_config ovmf_config =  {
     .bios_info_setup = ovmf_setup_bios_info,
     .bios_info_finish = ovmf_finish_bios_info,
 
-    .e820_setup = NULL,
+    .e820_setup = ovmf_setup_e820,
 
     .acpi_build_tables = ovmf_acpi_build_tables,
     .create_mp_tables = NULL,
-- 
1.7.10.4

Re: [PATCH RFC 0/4] hvmloader: make OVMF work with Xen

[Ian Campbell]

On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> This is the Xen side patches.
> 
> The important bits are setting up ovmf_info and E820 map. I'm particular
> interested what to be included in ovmf_info.

Follow the lead of seabios_info?

In any case you should design things to be extensible in the future,
since even if we think really hard now inevitably something else will
come along. That way you can add exactly what is needed today and add
other stuff when it becomes necessary.

I'm going to review what is here, but I think we should wait until
agreement is reached with the OVMF folks about that side of thing before
committing anything.

> 
> Wei.
> 
> Wei Liu (4):
>   hvmloader/ovmf: increase blob size to 2MB
>   hvmloader/ovmf: show OVMF_BEGIN as bios address
>   hvmloader/ovmf: setup ovmf_info
>   hvmloader:ovmf: setup E820 map
> 
>  tools/firmware/hvmloader/ovmf.c |   58 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 

Re: [PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB

[Ian Campbell]

On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> The debug build of OVMF now can be as large as 2MB.

And smaller builds are happy to be loaded lower?

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/firmware/hvmloader/ovmf.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index ee4cbbf..5c83626 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -38,8 +38,8 @@
>  #define ROM_INCLUDE_OVMF
>  #include "roms.inc"
>  
> -#define OVMF_BEGIN              0xFFF00000ULL
> -#define OVMF_SIZE               0x00100000ULL
> +#define OVMF_BEGIN              0xFFE00000ULL
> +#define OVMF_SIZE               0x00200000ULL
>  #define OVMF_MAXOFFSET          0x000FFFFFULL
>  #define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
>  #define LOWCHUNK_BEGIN          0x000F0000

Re: [PATCH RFC 2/4] hvmloader/ovmf: show OVMF_BEGIN as bios address

[Ian Campbell]

On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

This could be applied irrespective of any discussion with the OVMF
folks.

Re: [PATCH RFC 3/4] hvmloader/ovmf: setup ovmf_info

[Ian Campbell]

On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> OVMF info contains E820 map allocated by hvmloader. This info is passed
> to OVMF to help it do proper initialization.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/firmware/hvmloader/ovmf.c |   39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index 7826095..c253083 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -46,10 +46,45 @@
>  #define LOWCHUNK_SIZE           0x00010000
>  #define LOWCHUNK_MAXOFFSET      0x0000FFFF
>  #define LOWCHUNK_END            (OVMF_BEGIN + OVMF_SIZE)
> +#define OVMF_INFO_PHYSICAL_ADDRESS 0X00001000
>  
>  extern unsigned char dsdt_anycpu[];
>  extern int dsdt_anycpu_len;
>  
> +struct ovmf_info {
> +    char signature[11]; /* XenHVMOVMF\0 */
> +    uint8_t length;     /* Length of this struct */
> +    uint8_t checksum;   /* Set such that the sum over bytes 0..length == 0 */

You will need some explicit zero padding (to make the above add up to 16
bytes), otherwise there will be a hole in the struct which might cause
confusion when checksumming (i..e forgetting to sum the hole...)

Or you could add some nulls to the signature.

I wonder if we could share some/all of this with the Seabios code, just
with a different signature and potentially the info address?

> +    /*
> +     * Physical address of the e820 table, contains e820_nr entries.
> +     */
> +    uint32_t e820;
> +    uint32_t e820_nr;
> +} __attribute__ ((packed));
> +
> +static void ovmf_setup_bios_info(void)
> +{
> +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> +
> +    memset(info, 0, sizeof(*info));
> +
> +    memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature));
> +    info->length = sizeof(*info);
> +}
> +
> +static void ovmf_finish_bios_info(void)
> +{
> +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> +    uint32_t i;
> +    uint8_t checksum;
> +
> +    checksum = 0;
> +    for ( i = 0; i < info->length; i++ )
> +        checksum += ((uint8_t *)(info))[i];
> +
> +    info->checksum = -checksum;
> +}
> +
>  static void ovmf_load(const struct bios_config *config)
>  {
>      xen_pfn_t mfn;
> @@ -104,8 +139,8 @@ struct bios_config ovmf_config =  {
>  
>      .load_roms = 0,
>  
> -    .bios_info_setup = NULL,
> -    .bios_info_finish = NULL,
> +    .bios_info_setup = ovmf_setup_bios_info,
> +    .bios_info_finish = ovmf_finish_bios_info,
>  
>      .e820_setup = NULL,
>  

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Ian Campbell]

On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> E820 map will be used by OVMF to create memory map.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index c253083..52ccd0d 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
>          SMBIOS_PHYSICAL_END);
>  }
>  
> +static void ovmf_setup_e820(void)
> +{
> +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> +    info->e820 = (uint32_t)e820;
> +
> +    /* OVMF doesn't load bios image below 0x100000 */
> +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);

I'm afraid I don't understand that comment, what is it referring too?

> +    dump_e820_table(e820, info->e820_nr);
> +}
> +
>  struct bios_config ovmf_config =  {
>      .name = "OVMF",
>  
> @@ -142,7 +153,7 @@ struct bios_config ovmf_config =  {
>      .bios_info_setup = ovmf_setup_bios_info,
>      .bios_info_finish = ovmf_finish_bios_info,
>  
> -    .e820_setup = NULL,
> +    .e820_setup = ovmf_setup_e820,
>  
>      .acpi_build_tables = ovmf_acpi_build_tables,
>      .create_mp_tables = NULL,

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Wei Liu]

On Tue, Nov 19, 2013 at 05:56:28PM +0000, Ian Campbell wrote:
> On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > E820 map will be used by OVMF to create memory map.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > index c253083..52ccd0d 100644
> > --- a/tools/firmware/hvmloader/ovmf.c
> > +++ b/tools/firmware/hvmloader/ovmf.c
> > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> >          SMBIOS_PHYSICAL_END);
> >  }
> >  
> > +static void ovmf_setup_e820(void)
> > +{
> > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > +    info->e820 = (uint32_t)e820;
> > +
> > +    /* OVMF doesn't load bios image below 0x100000 */
> > +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
> 
> I'm afraid I don't understand that comment, what is it referring too?

Looking at build_e820_table, the third parameter is base address of bios
image. We already load OVMF to higher address, we only need to make
build_e820_table happy. Probably we should modify build_e820_table
instead?

Wei.

> 
> > +    dump_e820_table(e820, info->e820_nr);
> > +}
> > +
> >  struct bios_config ovmf_config =  {
> >      .name = "OVMF",
> >  
> > @@ -142,7 +153,7 @@ struct bios_config ovmf_config =  {
> >      .bios_info_setup = ovmf_setup_bios_info,
> >      .bios_info_finish = ovmf_finish_bios_info,
> >  
> > -    .e820_setup = NULL,
> > +    .e820_setup = ovmf_setup_e820,
> >  
> >      .acpi_build_tables = ovmf_acpi_build_tables,
> >      .create_mp_tables = NULL,
> 

Re: [PATCH RFC 3/4] hvmloader/ovmf: setup ovmf_info

[Wei Liu]

On Tue, Nov 19, 2013 at 05:54:36PM +0000, Ian Campbell wrote:
> On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > OVMF info contains E820 map allocated by hvmloader. This info is passed
> > to OVMF to help it do proper initialization.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/firmware/hvmloader/ovmf.c |   39 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 37 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > index 7826095..c253083 100644
> > --- a/tools/firmware/hvmloader/ovmf.c
> > +++ b/tools/firmware/hvmloader/ovmf.c
> > @@ -46,10 +46,45 @@
> >  #define LOWCHUNK_SIZE           0x00010000
> >  #define LOWCHUNK_MAXOFFSET      0x0000FFFF
> >  #define LOWCHUNK_END            (OVMF_BEGIN + OVMF_SIZE)
> > +#define OVMF_INFO_PHYSICAL_ADDRESS 0X00001000
> >  
> >  extern unsigned char dsdt_anycpu[];
> >  extern int dsdt_anycpu_len;
> >  
> > +struct ovmf_info {
> > +    char signature[11]; /* XenHVMOVMF\0 */
> > +    uint8_t length;     /* Length of this struct */
> > +    uint8_t checksum;   /* Set such that the sum over bytes 0..length == 0 */
> 
> You will need some explicit zero padding (to make the above add up to 16
> bytes), otherwise there will be a hole in the struct which might cause
> confusion when checksumming (i..e forgetting to sum the hole...)
> 
> Or you could add some nulls to the signature.
> 

OK.

> I wonder if we could share some/all of this with the Seabios code, just
> with a different signature and potentially the info address?
> 

Yes, we can, but that will also make things less flexible. We need to
100% sure this sturcture will always be the same for Seabios and OVMF.

Wei.

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Konrad Rzeszutek Wilk]

On Fri, Nov 15, 2013 at 03:59:25PM +0000, Wei Liu wrote:
> E820 map will be used by OVMF to create memory map.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index c253083..52ccd0d 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
>          SMBIOS_PHYSICAL_END);
>  }
>  
> +static void ovmf_setup_e820(void)
> +{
> +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);

16? Why not 128 (the default in most kernels)?

> +    info->e820 = (uint32_t)e820;
> +
> +    /* OVMF doesn't load bios image below 0x100000 */
> +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
> +    dump_e820_table(e820, info->e820_nr);
> +}
> +
>  struct bios_config ovmf_config =  {
>      .name = "OVMF",
>  
> @@ -142,7 +153,7 @@ struct bios_config ovmf_config =  {
>      .bios_info_setup = ovmf_setup_bios_info,
>      .bios_info_finish = ovmf_finish_bios_info,
>  
> -    .e820_setup = NULL,
> +    .e820_setup = ovmf_setup_e820,
>  
>      .acpi_build_tables = ovmf_acpi_build_tables,
>      .create_mp_tables = NULL,
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Wei Liu]

On Tue, Nov 19, 2013 at 02:56:31PM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 15, 2013 at 03:59:25PM +0000, Wei Liu wrote:
> > E820 map will be used by OVMF to create memory map.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > index c253083..52ccd0d 100644
> > --- a/tools/firmware/hvmloader/ovmf.c
> > +++ b/tools/firmware/hvmloader/ovmf.c
> > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> >          SMBIOS_PHYSICAL_END);
> >  }
> >  
> > +static void ovmf_setup_e820(void)
> > +{
> > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> 
> 16? Why not 128 (the default in most kernels)?

Aye, sir. :-)

Wei.

Re: [PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB

[Wei Liu]

On Tue, Nov 19, 2013 at 05:49:15PM +0000, Ian Campbell wrote:
> On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > The debug build of OVMF now can be as large as 2MB.
> 
> And smaller builds are happy to be loaded lower?

Yes, 1MB build (release build) works too.

> 
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/firmware/hvmloader/ovmf.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > index ee4cbbf..5c83626 100644
> > --- a/tools/firmware/hvmloader/ovmf.c
> > +++ b/tools/firmware/hvmloader/ovmf.c
> > @@ -38,8 +38,8 @@
> >  #define ROM_INCLUDE_OVMF
> >  #include "roms.inc"
> >  
> > -#define OVMF_BEGIN              0xFFF00000ULL
> > -#define OVMF_SIZE               0x00100000ULL
> > +#define OVMF_BEGIN              0xFFE00000ULL
> > +#define OVMF_SIZE               0x00200000ULL
> >  #define OVMF_MAXOFFSET          0x000FFFFFULL
> >  #define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
> >  #define LOWCHUNK_BEGIN          0x000F0000
> 

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Ian Campbell]

On Tue, 2013-11-19 at 18:01 +0000, Wei Liu wrote:
> On Tue, Nov 19, 2013 at 05:56:28PM +0000, Ian Campbell wrote:
> > On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > > E820 map will be used by OVMF to create memory map.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > index c253083..52ccd0d 100644
> > > --- a/tools/firmware/hvmloader/ovmf.c
> > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > >          SMBIOS_PHYSICAL_END);
> > >  }
> > >  
> > > +static void ovmf_setup_e820(void)
> > > +{
> > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > > +    info->e820 = (uint32_t)e820;
> > > +
> > > +    /* OVMF doesn't load bios image below 0x100000 */
> > > +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
> > 
> > I'm afraid I don't understand that comment, what is it referring too?
> 
> Looking at build_e820_table, the third parameter is base address of bios
> image. We already load OVMF to higher address, we only need to make
> build_e820_table happy. Probably we should modify build_e820_table
> instead?

What about the stuff at LOWCHUNK_BEGIN?

> 
> Wei.
> 
> > 
> > > +    dump_e820_table(e820, info->e820_nr);
> > > +}
> > > +
> > >  struct bios_config ovmf_config =  {
> > >      .name = "OVMF",
> > >  
> > > @@ -142,7 +153,7 @@ struct bios_config ovmf_config =  {
> > >      .bios_info_setup = ovmf_setup_bios_info,
> > >      .bios_info_finish = ovmf_finish_bios_info,
> > >  
> > > -    .e820_setup = NULL,
> > > +    .e820_setup = ovmf_setup_e820,
> > >  
> > >      .acpi_build_tables = ovmf_acpi_build_tables,
> > >      .create_mp_tables = NULL,
> > 

Re: [PATCH RFC 3/4] hvmloader/ovmf: setup ovmf_info

[Ian Campbell]

On Tue, 2013-11-19 at 18:03 +0000, Wei Liu wrote:
> On Tue, Nov 19, 2013 at 05:54:36PM +0000, Ian Campbell wrote:
> > I wonder if we could share some/all of this with the Seabios code, just
> > with a different signature and potentially the info address?
> > 
> 
> Yes, we can, but that will also make things less flexible. We need to
> 100% sure this sturcture will always be the same for Seabios and OVMF.

We can always split things back up again if they start to differ
considerably. I'd expect them to remain mostly identical for the time
being, since they mostly contain pretty well defined stuff.

Ian

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Ian Campbell]

On Tue, 2013-11-19 at 20:05 +0000, Wei Liu wrote:
> On Tue, Nov 19, 2013 at 02:56:31PM -0500, Konrad Rzeszutek Wilk wrote:
> > On Fri, Nov 15, 2013 at 03:59:25PM +0000, Wei Liu wrote:
> > > E820 map will be used by OVMF to create memory map.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > index c253083..52ccd0d 100644
> > > --- a/tools/firmware/hvmloader/ovmf.c
> > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > >          SMBIOS_PHYSICAL_END);
> > >  }
> > >  
> > > +static void ovmf_setup_e820(void)
> > > +{
> > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > 
> > 16? Why not 128 (the default in most kernels)?
> 
> Aye, sir. :-)

This is an internal datastructure which hvmloader generates an e820
into, it only needs to be large enough to hold whatever hvmloader
currently generates and not the theoretical maximum.

I think 16 is more than sufficient for now. In fact by looking at
build_e820_table it seems that it currently generates at most 9 entries.

If you really wanted to change something here then either expose a
suitable table size from e820.[ch] (e.g. as a #define) to
{seabios,ovmf}.c or pass in the allocated size and validate it against
nr in build_e820_table.

Ian.

Re: [PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB

[Ian Campbell]

On Tue, 2013-11-19 at 20:49 +0000, Wei Liu wrote:
> On Tue, Nov 19, 2013 at 05:49:15PM +0000, Ian Campbell wrote:
> > On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > > The debug build of OVMF now can be as large as 2MB.
> > 
> > And smaller builds are happy to be loaded lower?
> 
> Yes, 1MB build (release build) works too.

OK, thanks for confirming.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although you may also wish to consider basing the address on
0x100000000ULL-sizeof(ovmf). Perhaps with suitable rounding down.

> 
> > 
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/firmware/hvmloader/ovmf.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > index ee4cbbf..5c83626 100644
> > > --- a/tools/firmware/hvmloader/ovmf.c
> > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > @@ -38,8 +38,8 @@
> > >  #define ROM_INCLUDE_OVMF
> > >  #include "roms.inc"
> > >  
> > > -#define OVMF_BEGIN              0xFFF00000ULL
> > > -#define OVMF_SIZE               0x00100000ULL
> > > +#define OVMF_BEGIN              0xFFE00000ULL
> > > +#define OVMF_SIZE               0x00200000ULL
> > >  #define OVMF_MAXOFFSET          0x000FFFFFULL
> > >  #define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
> > >  #define LOWCHUNK_BEGIN          0x000F0000
> > 

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Wei Liu]

On Wed, Nov 20, 2013 at 10:07:08AM +0000, Ian Campbell wrote:
> On Tue, 2013-11-19 at 18:01 +0000, Wei Liu wrote:
> > On Tue, Nov 19, 2013 at 05:56:28PM +0000, Ian Campbell wrote:
> > > On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > > > E820 map will be used by OVMF to create memory map.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > > index c253083..52ccd0d 100644
> > > > --- a/tools/firmware/hvmloader/ovmf.c
> > > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > > >          SMBIOS_PHYSICAL_END);
> > > >  }
> > > >  
> > > > +static void ovmf_setup_e820(void)
> > > > +{
> > > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > > > +    info->e820 = (uint32_t)e820;
> > > > +
> > > > +    /* OVMF doesn't load bios image below 0x100000 */
> > > > +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
> > > 
> > > I'm afraid I don't understand that comment, what is it referring too?
> > 
> > Looking at build_e820_table, the third parameter is base address of bios
> > image. We already load OVMF to higher address, we only need to make
> > build_e820_table happy. Probably we should modify build_e820_table
> > instead?
> 
> What about the stuff at LOWCHUNK_BEGIN?
> 

Only RAM in E820 map is revelant in OVMF, all other entries
are unparsed. And 0xA0000 - 0x1A0000 is automatically reserved in OVMF
code.

Wei.

> > 
> > Wei.
> > 
> > > 
> > > > +    dump_e820_table(e820, info->e820_nr);
> > > > +}
> > > > +
> > > >  struct bios_config ovmf_config =  {
> > > >      .name = "OVMF",
> > > >  
> > > > @@ -142,7 +153,7 @@ struct bios_config ovmf_config =  {
> > > >      .bios_info_setup = ovmf_setup_bios_info,
> > > >      .bios_info_finish = ovmf_finish_bios_info,
> > > >  
> > > > -    .e820_setup = NULL,
> > > > +    .e820_setup = ovmf_setup_e820,
> > > >  
> > > >      .acpi_build_tables = ovmf_acpi_build_tables,
> > > >      .create_mp_tables = NULL,
> > > 
> 

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Ian Campbell]

On Wed, 2013-11-20 at 11:49 +0000, Wei Liu wrote:
> On Wed, Nov 20, 2013 at 10:07:08AM +0000, Ian Campbell wrote:
> > On Tue, 2013-11-19 at 18:01 +0000, Wei Liu wrote:
> > > On Tue, Nov 19, 2013 at 05:56:28PM +0000, Ian Campbell wrote:
> > > > On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > > > > E820 map will be used by OVMF to create memory map.
> > > > > 
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > > ---
> > > > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > > > index c253083..52ccd0d 100644
> > > > > --- a/tools/firmware/hvmloader/ovmf.c
> > > > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > > > >          SMBIOS_PHYSICAL_END);
> > > > >  }
> > > > >  
> > > > > +static void ovmf_setup_e820(void)
> > > > > +{
> > > > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > > > > +    info->e820 = (uint32_t)e820;
> > > > > +
> > > > > +    /* OVMF doesn't load bios image below 0x100000 */
> > > > > +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
> > > > 
> > > > I'm afraid I don't understand that comment, what is it referring too?
> > > 
> > > Looking at build_e820_table, the third parameter is base address of bios
> > > image. We already load OVMF to higher address, we only need to make
> > > build_e820_table happy. Probably we should modify build_e820_table
> > > instead?
> > 
> > What about the stuff at LOWCHUNK_BEGIN?
> > 
> 
> Only RAM in E820 map is revelant in OVMF, all other entries
> are unparsed. And 0xA0000 - 0x1A0000 is automatically reserved in OVMF
> code.

Regardless of what the OVMF code does I think the e820 table which
hvmloader passes through should attempt to accurately describe what
hvmloader has done.

Ian.

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Wei Liu]

On Wed, Nov 20, 2013 at 11:51:50AM +0000, Ian Campbell wrote:
> On Wed, 2013-11-20 at 11:49 +0000, Wei Liu wrote:
> > On Wed, Nov 20, 2013 at 10:07:08AM +0000, Ian Campbell wrote:
> > > On Tue, 2013-11-19 at 18:01 +0000, Wei Liu wrote:
> > > > On Tue, Nov 19, 2013 at 05:56:28PM +0000, Ian Campbell wrote:
> > > > > On Fri, 2013-11-15 at 15:59 +0000, Wei Liu wrote:
> > > > > > E820 map will be used by OVMF to create memory map.
> > > > > > 
> > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > > > ---
> > > > > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > > > > index c253083..52ccd0d 100644
> > > > > > --- a/tools/firmware/hvmloader/ovmf.c
> > > > > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > > > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > > > > >          SMBIOS_PHYSICAL_END);
> > > > > >  }
> > > > > >  
> > > > > > +static void ovmf_setup_e820(void)
> > > > > > +{
> > > > > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > > > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > > > > > +    info->e820 = (uint32_t)e820;
> > > > > > +
> > > > > > +    /* OVMF doesn't load bios image below 0x100000 */
> > > > > > +    info->e820_nr = build_e820_table(e820, 0, 0xfffff);
> > > > > 
> > > > > I'm afraid I don't understand that comment, what is it referring too?
> > > > 
> > > > Looking at build_e820_table, the third parameter is base address of bios
> > > > image. We already load OVMF to higher address, we only need to make
> > > > build_e820_table happy. Probably we should modify build_e820_table
> > > > instead?
> > > 
> > > What about the stuff at LOWCHUNK_BEGIN?
> > > 
> > 
> > Only RAM in E820 map is revelant in OVMF, all other entries
> > are unparsed. And 0xA0000 - 0x1A0000 is automatically reserved in OVMF
> > code.
> 
> Regardless of what the OVMF code does I think the e820 table which
> hvmloader passes through should attempt to accurately describe what
> hvmloader has done.
> 

It is a bit confusing that SeaBIOS codes is relying on SeaBIOS to
reserve certain region. I just followed suit.

In any case, I think you make a good point. I will add that region to
E820 map.

Wei.

> Ian.
> 

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Ian Campbell]

On Wed, 2013-11-20 at 11:58 +0000, Wei Liu wrote:
> On Wed, Nov 20, 2013 at 11:51:50AM +0000, Ian Campbell wrote:

> > Regardless of what the OVMF code does I think the e820 table which
> > hvmloader passes through should attempt to accurately describe what
> > hvmloader has done.
> > 
> 
> It is a bit confusing that SeaBIOS codes is relying on SeaBIOS to
> reserve certain region. I just followed suit.

Is it? The hvmloader seabios bindings are calling e820_build_table with
"0x100000-sizeof(seabios)" which is supposed to reserve the BIOS region.
(aside: that really ought to be ->bios_address!)

Or is there some other region which hvmloader sets up which needs
reserving?

It's ok for the BIOS to end up reserving more, based on the setup which
it does, of course. I think that is what the comment is referring too --
the fact that SeaBIOS reserves what it allocates. This is in contrast to
ROMBIOS which assumes hvmloader will create an e820 which it likes -- a
relic of the previous tight integration between hvmloader and seabios.

Ian.

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Wei Liu]

On Wed, Nov 20, 2013 at 12:11:25PM +0000, Ian Campbell wrote:
> On Wed, 2013-11-20 at 11:58 +0000, Wei Liu wrote:
> > On Wed, Nov 20, 2013 at 11:51:50AM +0000, Ian Campbell wrote:
> 
> > > Regardless of what the OVMF code does I think the e820 table which
> > > hvmloader passes through should attempt to accurately describe what
> > > hvmloader has done.
> > > 
> > 
> > It is a bit confusing that SeaBIOS codes is relying on SeaBIOS to
> > reserve certain region. I just followed suit.
> 
> Is it? The hvmloader seabios bindings are calling e820_build_table with
> "0x100000-sizeof(seabios)" which is supposed to reserve the BIOS region.
> (aside: that really ought to be ->bios_address!)
> 

The comment above the call to build_e820_table, "SeaBIOS reserves memory
in e820 as necessary so no low reservation".

> Or is there some other region which hvmloader sets up which needs
> reserving?
> 
> It's ok for the BIOS to end up reserving more, based on the setup which
> it does, of course. I think that is what the comment is referring too --
> the fact that SeaBIOS reserves what it allocates. This is in contrast to
> ROMBIOS which assumes hvmloader will create an e820 which it likes -- a
> relic of the previous tight integration between hvmloader and seabios.
> 

OK, I misread that comment then.

Wei.

> Ian.

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Konrad Rzeszutek Wilk]

On Wed, Nov 20, 2013 at 10:12:11AM +0000, Ian Campbell wrote:
> On Tue, 2013-11-19 at 20:05 +0000, Wei Liu wrote:
> > On Tue, Nov 19, 2013 at 02:56:31PM -0500, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Nov 15, 2013 at 03:59:25PM +0000, Wei Liu wrote:
> > > > E820 map will be used by OVMF to create memory map.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > > index c253083..52ccd0d 100644
> > > > --- a/tools/firmware/hvmloader/ovmf.c
> > > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > > >          SMBIOS_PHYSICAL_END);
> > > >  }
> > > >  
> > > > +static void ovmf_setup_e820(void)
> > > > +{
> > > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > > 
> > > 16? Why not 128 (the default in most kernels)?
> > 
> > Aye, sir. :-)
> 
> This is an internal datastructure which hvmloader generates an e820
> into, it only needs to be large enough to hold whatever hvmloader
> currently generates and not the theoretical maximum.
> 
> I think 16 is more than sufficient for now. In fact by looking at
> build_e820_table it seems that it currently generates at most 9 entries.
> 
> If you really wanted to change something here then either expose a
> suitable table size from e820.[ch] (e.g. as a #define) to
> {seabios,ovmf}.c or pass in the allocated size and validate it against
> nr in build_e820_table.

OK, I was thinking to make e820_hole parameter work with HVM. Which would
mean that the E820 can be up to 128.

I can do the right change to "allocated size and validate it against
nr in build_e820_table" when I get to this point.
> 
> Ian.
> 

Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map

[Ian Campbell]

On Wed, 2013-11-20 at 09:04 -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 20, 2013 at 10:12:11AM +0000, Ian Campbell wrote:
> > On Tue, 2013-11-19 at 20:05 +0000, Wei Liu wrote:
> > > On Tue, Nov 19, 2013 at 02:56:31PM -0500, Konrad Rzeszutek Wilk wrote:
> > > > On Fri, Nov 15, 2013 at 03:59:25PM +0000, Wei Liu wrote:
> > > > > E820 map will be used by OVMF to create memory map.
> > > > > 
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > > ---
> > > > >  tools/firmware/hvmloader/ovmf.c |   13 ++++++++++++-
> > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> > > > > index c253083..52ccd0d 100644
> > > > > --- a/tools/firmware/hvmloader/ovmf.c
> > > > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > > > @@ -128,6 +128,17 @@ static void ovmf_create_smbios_tables(void)
> > > > >          SMBIOS_PHYSICAL_END);
> > > > >  }
> > > > >  
> > > > > +static void ovmf_setup_e820(void)
> > > > > +{
> > > > > +    struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> > > > > +    struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > > > 
> > > > 16? Why not 128 (the default in most kernels)?
> > > 
> > > Aye, sir. :-)
> > 
> > This is an internal datastructure which hvmloader generates an e820
> > into, it only needs to be large enough to hold whatever hvmloader
> > currently generates and not the theoretical maximum.
> > 
> > I think 16 is more than sufficient for now. In fact by looking at
> > build_e820_table it seems that it currently generates at most 9 entries.
> > 
> > If you really wanted to change something here then either expose a
> > suitable table size from e820.[ch] (e.g. as a #define) to
> > {seabios,ovmf}.c or pass in the allocated size and validate it against
> > nr in build_e820_table.
> 
> OK, I was thinking to make e820_hole parameter work with HVM. Which would
> mean that the E820 can be up to 128.
> 
> I can do the right change to "allocated size and validate it against
> nr in build_e820_table" when I get to this point.

Yes, please lets do it when it is needed not yet.

Ian/.