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
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
Wei Liu
2013-Nov-15 15:59 UTC
[PATCH RFC 2/4] hvmloader/ovmf: show OVMF_BEGIN as bios address
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
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
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
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(-) >
Ian Campbell
2013-Nov-19 17:49 UTC
Re: [PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB
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
Ian Campbell
2013-Nov-19 17:51 UTC
Re: [PATCH RFC 2/4] hvmloader/ovmf: show OVMF_BEGIN as bios address
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.
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, >
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,
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, >
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.
Konrad Rzeszutek Wilk
2013-Nov-19 19:56 UTC
Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map
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
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.
Wei Liu
2013-Nov-19 20:49 UTC
Re: [PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB
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 >
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, > >
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
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.
Ian Campbell
2013-Nov-20 10:13 UTC
Re: [PATCH RFC 1/4] hvmloader/ovmf: increase blob size to 2MB
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 > >
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, > > > >
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.
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. >
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.
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.
Konrad Rzeszutek Wilk
2013-Nov-20 14:04 UTC
Re: [PATCH RFC 4/4] hvmloader:ovmf: setup E820 map
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. >
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/.