* [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 10:48 ` Ian Jackson
2013-06-21 10:55 ` Stefano Stabellini
2013-06-21 10:46 ` [PATCH v4 2/8] hvmloader: Make the printfs more informative George Dunlap
` (6 subsequent siblings)
7 siblings, 2 replies; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
The printf() available to hvmloader does not handle 64-bit data types;
manually break them down as two 32-bit strings.
v4:
- Make macros for the requisite format and bit shifting
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 11 +++++++----
tools/firmware/hvmloader/util.h | 2 ++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index c78d4d3..c1cb1e9 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -290,8 +290,9 @@ void pci_setup(void)
if ( (base < resource->base) || (base > resource->max) )
{
- printf("pci dev %02x:%x bar %02x size %llx: no space for "
- "resource!\n", devfn>>3, devfn&7, bar_reg, bar_sz);
+ printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
+ "resource!\n", devfn>>3, devfn&7, bar_reg,
+ PRIllx_arg(bar_sz));
continue;
}
@@ -300,8 +301,10 @@ void pci_setup(void)
pci_writel(devfn, bar_reg, bar_data);
if (using_64bar)
pci_writel(devfn, bar_reg + 4, bar_data_upper);
- printf("pci dev %02x:%x bar %02x size %llx: %08x\n",
- devfn>>3, devfn&7, bar_reg, bar_sz, bar_data);
+ printf("pci dev %02x:%x bar %02x size "PRIllx": %08x\n",
+ devfn>>3, devfn&7, bar_reg,
+ PRIllx_arg(bar_sz),
+ bar_data);
/* Now enable the memory or I/O mapping. */
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 7913259..9ccb905 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -168,6 +168,8 @@ void byte_to_hex(char *digits, uint8_t byte);
void uuid_to_string(char *dest, uint8_t *uuid);
/* Debug output */
+#define PRIllx "%x%08x"
+#define PRIllx_arg(ll) (uint32_t)((ll)>>32), (uint32_t)(ll)
int printf(const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
int vprintf(const char *fmt, va_list ap);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments
2013-06-21 10:46 ` [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments George Dunlap
@ 2013-06-21 10:48 ` Ian Jackson
2013-06-21 11:20 ` Keir Fraser
2013-06-21 10:55 ` Stefano Stabellini
1 sibling, 1 reply; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 10:48 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments"):
> The printf() available to hvmloader does not handle 64-bit data types;
> manually break them down as two 32-bit strings.
>
> v4:
> - Make macros for the requisite format and bit shifting
This is an improvement.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments
2013-06-21 10:48 ` Ian Jackson
@ 2013-06-21 11:20 ` Keir Fraser
0 siblings, 0 replies; 29+ messages in thread
From: Keir Fraser @ 2013-06-21 11:20 UTC (permalink / raw)
To: Ian Jackson, George Dunlap
Cc: Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
On 21/06/2013 11:48, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:
> George Dunlap writes ("[PATCH v4 1/8] hvmloader: Remove all 64-bit print
> arguments"):
>> The printf() available to hvmloader does not handle 64-bit data types;
>> manually break them down as two 32-bit strings.
>>
>> v4:
>> - Make macros for the requisite format and bit shifting
>
> This is an improvement.
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Keir Fraser <keir@xen.org>
I like it because it makes it easy to go implement %llx properly later. I
think it's dumb we're not doing that now to be honest, we have code in Xen
that could be pinched.
-- Keir
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments
2013-06-21 10:46 ` [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments George Dunlap
2013-06-21 10:48 ` Ian Jackson
@ 2013-06-21 10:55 ` Stefano Stabellini
1 sibling, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2013-06-21 10:55 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Ian Campbell, Hanweidong, xen-devel,
Stefano Stabellini, Ian Jackson
On Fri, 21 Jun 2013, George Dunlap wrote:
> The printf() available to hvmloader does not handle 64-bit data types;
> manually break them down as two 32-bit strings.
>
> v4:
> - Make macros for the requisite format and bit shifting
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> CC: Keir Fraser <keir@xen.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> tools/firmware/hvmloader/pci.c | 11 +++++++----
> tools/firmware/hvmloader/util.h | 2 ++
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index c78d4d3..c1cb1e9 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -290,8 +290,9 @@ void pci_setup(void)
>
> if ( (base < resource->base) || (base > resource->max) )
> {
> - printf("pci dev %02x:%x bar %02x size %llx: no space for "
> - "resource!\n", devfn>>3, devfn&7, bar_reg, bar_sz);
> + printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
> + "resource!\n", devfn>>3, devfn&7, bar_reg,
> + PRIllx_arg(bar_sz));
> continue;
> }
>
> @@ -300,8 +301,10 @@ void pci_setup(void)
> pci_writel(devfn, bar_reg, bar_data);
> if (using_64bar)
> pci_writel(devfn, bar_reg + 4, bar_data_upper);
> - printf("pci dev %02x:%x bar %02x size %llx: %08x\n",
> - devfn>>3, devfn&7, bar_reg, bar_sz, bar_data);
> + printf("pci dev %02x:%x bar %02x size "PRIllx": %08x\n",
> + devfn>>3, devfn&7, bar_reg,
> + PRIllx_arg(bar_sz),
> + bar_data);
>
>
> /* Now enable the memory or I/O mapping. */
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 7913259..9ccb905 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -168,6 +168,8 @@ void byte_to_hex(char *digits, uint8_t byte);
> void uuid_to_string(char *dest, uint8_t *uuid);
>
> /* Debug output */
> +#define PRIllx "%x%08x"
> +#define PRIllx_arg(ll) (uint32_t)((ll)>>32), (uint32_t)(ll)
> int printf(const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
> int vprintf(const char *fmt, va_list ap);
>
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 2/8] hvmloader: Make the printfs more informative
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
2013-06-21 10:46 ` [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 10:49 ` Ian Jackson
2013-06-21 10:57 ` Stefano Stabellini
2013-06-21 10:46 ` [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G George Dunlap
` (5 subsequent siblings)
7 siblings, 2 replies; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
* Warn that you're relocating some BARs to 64-bit
* Warn that you're relocating guest pages, and how many
* Include upper 32-bits of the base register when printing the bar
placement info
v4:
- Move message about relocating guest pages into loop, include number
of pages and guest paddr
- Fixed minor brace style issue
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index c1cb1e9..44168e2 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -214,7 +214,11 @@ void pci_setup(void)
pci_mem_start <<= 1;
if ( (pci_mem_start << 1) != 0 )
+ {
+ printf("Low MMIO hole not large enough for all devices,"
+ " relocating some BARs to 64-bit\n");
bar64_relocate = 1;
+ }
/* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
@@ -227,6 +231,11 @@ void pci_setup(void)
if ( hvm_info->high_mem_pgend == 0 )
hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
hvm_info->low_mem_pgend -= nr_pages;
+ printf("Relocating 0x%x pages from "PRIllx" to "PRIllx\
+ " for lowmem MMIO hole\n",
+ nr_pages,
+ PRIllx_arg(((uint64_t)hvm_info->low_mem_pgend)<<PAGE_SHIFT),
+ PRIllx_arg(((uint64_t)hvm_info->high_mem_pgend)<<PAGE_SHIFT));
xatp.domid = DOMID_SELF;
xatp.space = XENMAPSPACE_gmfn_range;
xatp.idx = hvm_info->low_mem_pgend;
@@ -301,10 +310,10 @@ void pci_setup(void)
pci_writel(devfn, bar_reg, bar_data);
if (using_64bar)
pci_writel(devfn, bar_reg + 4, bar_data_upper);
- printf("pci dev %02x:%x bar %02x size "PRIllx": %08x\n",
+ printf("pci dev %02x:%x bar %02x size "PRIllx": %x%08x\n",
devfn>>3, devfn&7, bar_reg,
PRIllx_arg(bar_sz),
- bar_data);
+ bar_data_upper, bar_data);
/* Now enable the memory or I/O mapping. */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/8] hvmloader: Make the printfs more informative
2013-06-21 10:46 ` [PATCH v4 2/8] hvmloader: Make the printfs more informative George Dunlap
@ 2013-06-21 10:49 ` Ian Jackson
2013-06-21 10:57 ` Stefano Stabellini
1 sibling, 0 replies; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 10:49 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 2/8] hvmloader: Make the printfs more informative"):
> * Warn that you're relocating some BARs to 64-bit
>
> * Warn that you're relocating guest pages, and how many
>
> * Include upper 32-bits of the base register when printing the bar
> placement info
>
> v4:
> - Move message about relocating guest pages into loop, include number
> of pages and guest paddr
> - Fixed minor brace style issue
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/8] hvmloader: Make the printfs more informative
2013-06-21 10:46 ` [PATCH v4 2/8] hvmloader: Make the printfs more informative George Dunlap
2013-06-21 10:49 ` Ian Jackson
@ 2013-06-21 10:57 ` Stefano Stabellini
1 sibling, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2013-06-21 10:57 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Ian Campbell, Hanweidong, xen-devel,
Stefano Stabellini, Ian Jackson
On Fri, 21 Jun 2013, George Dunlap wrote:
> * Warn that you're relocating some BARs to 64-bit
>
> * Warn that you're relocating guest pages, and how many
>
> * Include upper 32-bits of the base register when printing the bar
> placement info
>
> v4:
> - Move message about relocating guest pages into loop, include number
> of pages and guest paddr
> - Fixed minor brace style issue
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> CC: Keir Fraser <keir@xen.org>
> ---
> tools/firmware/hvmloader/pci.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index c1cb1e9..44168e2 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -214,7 +214,11 @@ void pci_setup(void)
> pci_mem_start <<= 1;
>
> if ( (pci_mem_start << 1) != 0 )
> + {
> + printf("Low MMIO hole not large enough for all devices,"
> + " relocating some BARs to 64-bit\n");
> bar64_relocate = 1;
> + }
>
> /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
> while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
> @@ -227,6 +231,11 @@ void pci_setup(void)
> if ( hvm_info->high_mem_pgend == 0 )
> hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
> hvm_info->low_mem_pgend -= nr_pages;
> + printf("Relocating 0x%x pages from "PRIllx" to "PRIllx\
> + " for lowmem MMIO hole\n",
> + nr_pages,
> + PRIllx_arg(((uint64_t)hvm_info->low_mem_pgend)<<PAGE_SHIFT),
> + PRIllx_arg(((uint64_t)hvm_info->high_mem_pgend)<<PAGE_SHIFT));
> xatp.domid = DOMID_SELF;
> xatp.space = XENMAPSPACE_gmfn_range;
> xatp.idx = hvm_info->low_mem_pgend;
This wasn't exactly what I suggested but it's correct.
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> @@ -301,10 +310,10 @@ void pci_setup(void)
> pci_writel(devfn, bar_reg, bar_data);
> if (using_64bar)
> pci_writel(devfn, bar_reg + 4, bar_data_upper);
> - printf("pci dev %02x:%x bar %02x size "PRIllx": %08x\n",
> + printf("pci dev %02x:%x bar %02x size "PRIllx": %x%08x\n",
> devfn>>3, devfn&7, bar_reg,
> PRIllx_arg(bar_sz),
> - bar_data);
> + bar_data_upper, bar_data);
>
>
> /* Now enable the memory or I/O mapping. */
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
2013-06-21 10:46 ` [PATCH v4 1/8] hvmloader: Remove all 64-bit print arguments George Dunlap
2013-06-21 10:46 ` [PATCH v4 2/8] hvmloader: Make the printfs more informative George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 10:50 ` Ian Jackson
2013-06-21 11:11 ` Stefano Stabellini
2013-06-21 10:46 ` [PATCH v4 4/8] hvmloader: Fix check for needing a 64-bit bar George Dunlap
` (4 subsequent siblings)
7 siblings, 2 replies; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
hvmloader will read hvm_info->high_mem_pgend to calculate where to
start the highmem PCI region. However, if the guest does not have any
memory in the high region, this is set to zero, which will cause
hvmloader to use the "0" for the base of the highmem region, rather
than 1 << 32.
Check to see whether hvm_info->high_mem_pgend is set; if so, do the
normal calculation; otherwise, use 1<<32.
v4:
- Handle case where hfm_info->high_mem_pgend is non-zero but doesn't
point into high memory, throwing a warning.
Signed-off-by: Geore Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 44168e2..a3d03ed 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -246,7 +246,18 @@ void pci_setup(void)
hvm_info->high_mem_pgend += nr_pages;
}
- high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
+ high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
+ if ( high_mem_resource.base < 1ull << 32 )
+ {
+ if ( hvm_info->high_mem_pgend != 0 )
+ printf("WARNING: hvm_info->high_mem_pgend %x"
+ " does not point into high memory!",
+ hvm_info->high_mem_pgend);
+ high_mem_resource.base = 1ull << 32;
+ }
+ printf("%sRAM in high memory; setting high_mem resource base to "PRIllx"\n",
+ hvm_info->high_mem_pgend?"":"No ",
+ PRIllx_arg(high_mem_resource.base));
high_mem_resource.max = 1ull << cpu_phys_addr();
mem_resource.base = pci_mem_start;
mem_resource.max = pci_mem_end;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G
2013-06-21 10:46 ` [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G George Dunlap
@ 2013-06-21 10:50 ` Ian Jackson
2013-06-21 11:11 ` Stefano Stabellini
1 sibling, 0 replies; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 10:50 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G"):
> hvmloader will read hvm_info->high_mem_pgend to calculate where to
> start the highmem PCI region. However, if the guest does not have any
> memory in the high region, this is set to zero, which will cause
> hvmloader to use the "0" for the base of the highmem region, rather
> than 1 << 32.
>
> Check to see whether hvm_info->high_mem_pgend is set; if so, do the
> normal calculation; otherwise, use 1<<32.
>
> v4:
>
> - Handle case where hfm_info->high_mem_pgend is non-zero but doesn't
> point into high memory, throwing a warning.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G
2013-06-21 10:46 ` [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G George Dunlap
2013-06-21 10:50 ` Ian Jackson
@ 2013-06-21 11:11 ` Stefano Stabellini
1 sibling, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2013-06-21 11:11 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Ian Campbell, Hanweidong, xen-devel,
Stefano Stabellini, Ian Jackson
On Fri, 21 Jun 2013, George Dunlap wrote:
> hvmloader will read hvm_info->high_mem_pgend to calculate where to
> start the highmem PCI region. However, if the guest does not have any
> memory in the high region, this is set to zero, which will cause
> hvmloader to use the "0" for the base of the highmem region, rather
> than 1 << 32.
>
> Check to see whether hvm_info->high_mem_pgend is set; if so, do the
> normal calculation; otherwise, use 1<<32.
>
> v4:
>
> - Handle case where hfm_info->high_mem_pgend is non-zero but doesn't
> point into high memory, throwing a warning.
>
>
> Signed-off-by: Geore Dunlap <george.dunlap@eu.citrix.com>
^ ?
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> CC: Keir Fraser <keir@xen.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> tools/firmware/hvmloader/pci.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 44168e2..a3d03ed 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -246,7 +246,18 @@ void pci_setup(void)
> hvm_info->high_mem_pgend += nr_pages;
> }
>
> - high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
> + high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
> + if ( high_mem_resource.base < 1ull << 32 )
> + {
> + if ( hvm_info->high_mem_pgend != 0 )
> + printf("WARNING: hvm_info->high_mem_pgend %x"
> + " does not point into high memory!",
> + hvm_info->high_mem_pgend);
> + high_mem_resource.base = 1ull << 32;
> + }
> + printf("%sRAM in high memory; setting high_mem resource base to "PRIllx"\n",
> + hvm_info->high_mem_pgend?"":"No ",
> + PRIllx_arg(high_mem_resource.base));
> high_mem_resource.max = 1ull << cpu_phys_addr();
> mem_resource.base = pci_mem_start;
> mem_resource.max = pci_mem_end;
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 4/8] hvmloader: Fix check for needing a 64-bit bar
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
` (2 preceding siblings ...)
2013-06-21 10:46 ` [PATCH v4 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 10:51 ` Ian Jackson
2013-06-21 10:46 ` [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting George Dunlap
` (3 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
After attempting to resize the MMIO hole, the check to determine
whether there is a need to relocate BARs into 64-bit space checks the
specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
than checking whether the required MMIO will fit in the hole.
But even then it does it wrong: the polarity of the check is
backwards.
Check for the actual condition we care about (the sizeof the MMIO
hole) rather than checking for the loop exit condition.
v3:
- Move earlier in the series, before other functional changes
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index a3d03ed..6792ed4 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -213,7 +213,7 @@ void pci_setup(void)
((pci_mem_start << 1) != 0) )
pci_mem_start <<= 1;
- if ( (pci_mem_start << 1) != 0 )
+ if ( mmio_total > (pci_mem_end - pci_mem_start) )
{
printf("Low MMIO hole not large enough for all devices,"
" relocating some BARs to 64-bit\n");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 4/8] hvmloader: Fix check for needing a 64-bit bar
2013-06-21 10:46 ` [PATCH v4 4/8] hvmloader: Fix check for needing a 64-bit bar George Dunlap
@ 2013-06-21 10:51 ` Ian Jackson
0 siblings, 0 replies; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 10:51 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 4/8] hvmloader: Fix check for needing a 64-bit bar"):
> After attempting to resize the MMIO hole, the check to determine
> whether there is a need to relocate BARs into 64-bit space checks the
> specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
> than checking whether the required MMIO will fit in the hole.
>
> But even then it does it wrong: the polarity of the check is
> backwards.
>
> Check for the actual condition we care about (the sizeof the MMIO
> hole) rather than checking for the loop exit condition.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
` (3 preceding siblings ...)
2013-06-21 10:46 ` [PATCH v4 4/8] hvmloader: Fix check for needing a 64-bit bar George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 11:19 ` Ian Jackson
2013-06-21 10:46 ` [PATCH v4 6/8] hvmloader: Load large devices into high MMIO space as needed George Dunlap
` (2 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
When deciding whether to map a device in low MMIO space (<4GiB),
hvmloader compares it with "mmio_left", which is set to the size of
the low MMIO range (pci_mem_end - pci_mem_start). However, even if it
does map a device in high MMIO space, it still removes the size of its
BAR from mmio_left.
In reality we don't need to do a separate accounting of the low memory
available -- this can be calculated from mem_resource. Just get rid
of the variable and the duplicate accounting entirely. This will make
the code more robust.
v3:
- Use mem_resource values directly instead of doing duplicate
accounting
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 6792ed4..80eef76 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -42,7 +42,6 @@ void pci_setup(void)
uint32_t vga_devfn = 256;
uint16_t class, vendor_id, device_id;
unsigned int bar, pin, link, isa_irq;
- int64_t mmio_left;
/* Resources assignable to PCI devices via BARs. */
struct resource {
@@ -264,8 +263,6 @@ void pci_setup(void)
io_resource.base = 0xc000;
io_resource.max = 0x10000;
- mmio_left = pci_mem_end - pci_mem_start;
-
/* Assign iomem and ioport resources in descending order of size. */
for ( i = 0; i < nr_bars; i++ )
{
@@ -273,7 +270,8 @@ void pci_setup(void)
bar_reg = bars[i].bar_reg;
bar_sz = bars[i].bar_sz;
- using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left < bar_sz);
+ using_64bar = bars[i].is_64bar && bar64_relocate
+ && (bar_sz > (mem_resource.max - mem_resource.base));
bar_data = pci_readl(devfn, bar_reg);
if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
@@ -295,7 +293,6 @@ void pci_setup(void)
resource = &mem_resource;
bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
}
- mmio_left -= bar_sz;
}
else
{
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting
2013-06-21 10:46 ` [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting George Dunlap
@ 2013-06-21 11:19 ` Ian Jackson
2013-06-21 12:58 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 11:19 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting"):
> When deciding whether to map a device in low MMIO space (<4GiB),
> hvmloader compares it with "mmio_left", which is set to the size of
> the low MMIO range (pci_mem_end - pci_mem_start). However, even if it
> does map a device in high MMIO space, it still removes the size of its
> BAR from mmio_left.
>
> In reality we don't need to do a separate accounting of the low memory
> available -- this can be calculated from mem_resource. Just get rid
> of the variable and the duplicate accounting entirely. This will make
> the code more robust.
...
> - using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left < bar_sz);
> + using_64bar = bars[i].is_64bar && bar64_relocate
> + && (bar_sz > (mem_resource.max - mem_resource.base));
This is not entirely straightforward I think.
The actual calculation about whether it will actually fit, rather than
a precalculation of whether it is going to fit, is done here:
base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
bar_data |= (uint32_t)base;
bar_data_upper = (uint32_t)(base >> 32);
base += bar_sz;
if ( (base < resource->base) || (base > resource->max) )
[ ... doesn't fit ... ]
The first test rounds the base up to a multiple of bar_sz. I assume
that this is a requirement of the PCI spec.
(While I'm here I'll note that the (uint64_t) cast in that line is
unneccessary and confusing. If bar_sz weren't 64-bit this code would
be quite wrong, and putting that cast there suggests that it might not
be.)
In infer (from "bar_sz &= ~(bar_sz - 1)") that bar_sz is supposed to
be always a power of two. And we have devices in descending order of
size. So at least after the first device, this rounding does nothing.
But for the first device I think it may be possible for resource->base
not to be a multiple of the bar_sz, and in that case it might be that
the precalculation thinks it will fit when the actual placement
calculation doesn't.
Do you think this is possible ?
This is certainly excessively confusing. From a lack-of-regressions
point of view we are going to have to analyse it properly regardless
of whether we restructure it or not.
I would be tempted to suggest lifting the "base" etc. calculation into
a macro or function so that we can directly say
+ using_64bar = bars[i].is_64bar && bar64_relocate
+ && !try_allocate_resource(&mem_resource, &allocd, &new_base)
and later
- base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
- bar_data |= (uint32_t)base;
- bar_data_upper = (uint32_t)(base >> 32);
- base += bar_sz;
-
+ if ( !try_allocate_resource(resource, &allocd, &new_base) )
{
printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
"resource!\n", devfn>>3, devfn&7, bar_reg,
PRIllx_arg(bar_sz));
continue;
}
- resource->base = base;
+ resource->base = new_base;
+ bar_data |= (uint32_t)allocd;
+ bar_data_upper = (uint32_t)(allocd >> 32);
or something.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting
2013-06-21 11:19 ` Ian Jackson
@ 2013-06-21 12:58 ` Jan Beulich
2013-06-21 13:32 ` Ian Jackson
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2013-06-21 12:58 UTC (permalink / raw)
To: George Dunlap, Ian Jackson
Cc: Keir Fraser, xen-devel, Stefano Stabellini, Ian Campbell, Hanweidong
>>> On 21.06.13 at 13:19, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> George Dunlap writes ("[PATCH v4 5/8] hvmloader: Correct bug in low mmio
> region accounting"):
>> When deciding whether to map a device in low MMIO space (<4GiB),
>> hvmloader compares it with "mmio_left", which is set to the size of
>> the low MMIO range (pci_mem_end - pci_mem_start). However, even if it
>> does map a device in high MMIO space, it still removes the size of its
>> BAR from mmio_left.
>>
>> In reality we don't need to do a separate accounting of the low memory
>> available -- this can be calculated from mem_resource. Just get rid
>> of the variable and the duplicate accounting entirely. This will make
>> the code more robust.
> ...
>> - using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left < bar_sz);
>> + using_64bar = bars[i].is_64bar && bar64_relocate
>> + && (bar_sz > (mem_resource.max - mem_resource.base));
>
> This is not entirely straightforward I think.
>
> The actual calculation about whether it will actually fit, rather than
> a precalculation of whether it is going to fit, is done here:
>
> base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
> bar_data |= (uint32_t)base;
> bar_data_upper = (uint32_t)(base >> 32);
> base += bar_sz;
>
> if ( (base < resource->base) || (base > resource->max) )
> [ ... doesn't fit ... ]
>
> The first test rounds the base up to a multiple of bar_sz. I assume
> that this is a requirement of the PCI spec.
>
> (While I'm here I'll note that the (uint64_t) cast in that line is
> unneccessary and confusing. If bar_sz weren't 64-bit this code would
> be quite wrong, and putting that cast there suggests that it might not
> be.)
>
> In infer (from "bar_sz &= ~(bar_sz - 1)") that bar_sz is supposed to
> be always a power of two. And we have devices in descending order of
> size. So at least after the first device, this rounding does nothing.
>
> But for the first device I think it may be possible for resource->base
> not to be a multiple of the bar_sz, and in that case it might be that
> the precalculation thinks it will fit when the actual placement
> calculation doesn't.
>
> Do you think this is possible ?
This is possible only from an abstract perspective, not in reality:
PCI_MEM_START being 0x{f,e,c,8}0000000, PCI_MEM_END being
0xfc000000, and allocations starting with the biggest BARs
(where you already correctly noted that BARs are always a power
of 2 in size), the current base address can be misaligned only
when the BAR size is too large to fit anyway. In which case it'll
go into the space above 4Gb, and to that range the precalculation
doesn't apply.
Jan
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting
2013-06-21 12:58 ` Jan Beulich
@ 2013-06-21 13:32 ` Ian Jackson
2013-06-21 13:40 ` George Dunlap
0 siblings, 1 reply; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 13:32 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap, xen-devel,
Stefano Stabellini
Jan Beulich writes ("Re: [Xen-devel] [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting"):
> On 21.06.13 at 13:19, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > But for the first device I think it may be possible for resource->base
> > not to be a multiple of the bar_sz, and in that case it might be that
> > the precalculation thinks it will fit when the actual placement
> > calculation doesn't.
> >
> > Do you think this is possible ?
>
> This is possible only from an abstract perspective, not in reality:
> PCI_MEM_START being 0x{f,e,c,8}0000000, PCI_MEM_END being
> 0xfc000000, and allocations starting with the biggest BARs
> (where you already correctly noted that BARs are always a power
> of 2 in size), the current base address can be misaligned only
> when the BAR size is too large to fit anyway. In which case it'll
> go into the space above 4Gb, and to that range the precalculation
> doesn't apply.
Ah. Right. Err, OK. I'm convinced by this argument.
It's not a good reflection on the clarity of this code, though.
Perhaps, George, you could mention this issue in a comment or the
commit message.
But anyway, this, and 6/8,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting
2013-06-21 13:32 ` Ian Jackson
@ 2013-06-21 13:40 ` George Dunlap
0 siblings, 0 replies; 29+ messages in thread
From: George Dunlap @ 2013-06-21 13:40 UTC (permalink / raw)
To: Ian Jackson
Cc: Keir Fraser, Ian Campbell, Hanweidong, xen-devel,
Stefano Stabellini, Jan Beulich
On 21/06/13 14:32, Ian Jackson wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting"):
>> On 21.06.13 at 13:19, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>>> But for the first device I think it may be possible for resource->base
>>> not to be a multiple of the bar_sz, and in that case it might be that
>>> the precalculation thinks it will fit when the actual placement
>>> calculation doesn't.
>>>
>>> Do you think this is possible ?
>> This is possible only from an abstract perspective, not in reality:
>> PCI_MEM_START being 0x{f,e,c,8}0000000, PCI_MEM_END being
>> 0xfc000000, and allocations starting with the biggest BARs
>> (where you already correctly noted that BARs are always a power
>> of 2 in size), the current base address can be misaligned only
>> when the BAR size is too large to fit anyway. In which case it'll
>> go into the space above 4Gb, and to that range the precalculation
>> doesn't apply.
> Ah. Right. Err, OK. I'm convinced by this argument.
>
> It's not a good reflection on the clarity of this code, though.
> Perhaps, George, you could mention this issue in a comment or the
> commit message.
Yes, I think I shall. It is, as Jan says, correct at the present
moment, but it's not even clear whether that was by accident or by
design; even if it was by design, there's no guarantee it will remain so
in the future without at least a comment.
We may want to try to clean this up long-term, but I would really like
to investigate just punting this whole thing off to SeaBIOS, which is
being tested and maintained by the KVM folks.
> But anyway, this, and 6/8,
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Great, thanks.
Stefano pointed out some "development process" terminology leaking into
the comment on the last patch -- I'll clean that up, add in some
comments about the fragile accounting, and send v5. That should be it
for this series, I think.
-George
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 6/8] hvmloader: Load large devices into high MMIO space as needed
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
` (4 preceding siblings ...)
2013-06-21 10:46 ` [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 11:21 ` Ian Jackson
2013-06-21 10:46 ` [PATCH v4 7/8] hvmloader: Remove minimum size for BARs to relocate to 64-bit space George Dunlap
2013-06-21 10:46 ` [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole George Dunlap
7 siblings, 1 reply; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
Keep track of how much mmio space is left total, as well as the amount
of "low" MMIO space (<4GiB), and only load devices into high memory if
there is not enough low memory for the rest of the devices to fit.
Because devices are processed by size in order from large to small,
this should preferentially relocate devices with large BARs to 64-bit
space.
v3:
- Just use mmio_total rather than introducing a new variable.
- Port to using mem_resource directly rather than low_mmio_left
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 80eef76..3f368f3 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -270,8 +270,12 @@ void pci_setup(void)
bar_reg = bars[i].bar_reg;
bar_sz = bars[i].bar_sz;
+ /* Relocate to high memory if the total amount of MMIO needed
+ * is more than the low MMIO available. Because devices are
+ * processed in order of bar_sz, this will preferentially
+ * relocate larger devices to high memory first. */
using_64bar = bars[i].is_64bar && bar64_relocate
- && (bar_sz > (mem_resource.max - mem_resource.base));
+ && (mmio_total > (mem_resource.max - mem_resource.base));
bar_data = pci_readl(devfn, bar_reg);
if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
@@ -293,6 +297,7 @@ void pci_setup(void)
resource = &mem_resource;
bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
}
+ mmio_total -= bar_sz;
}
else
{
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/8] hvmloader: Load large devices into high MMIO space as needed
2013-06-21 10:46 ` [PATCH v4 6/8] hvmloader: Load large devices into high MMIO space as needed George Dunlap
@ 2013-06-21 11:21 ` Ian Jackson
0 siblings, 0 replies; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 11:21 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 6/8] hvmloader: Load large devices into high MMIO space as needed"):
> Keep track of how much mmio space is left total, as well as the amount
> of "low" MMIO space (<4GiB), and only load devices into high memory if
> there is not enough low memory for the rest of the devices to fit.
>
> Because devices are processed by size in order from large to small,
> this should preferentially relocate devices with large BARs to 64-bit
> space.
Does this have similar rounding/padding considerations as I discussed
in response to 5/8 ? I think it probably does...
In fact this one is worse because you calculate whether the first
(biggest) device will fit without considering its rounding, and then
allocate it with rounding.
Ian.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 7/8] hvmloader: Remove minimum size for BARs to relocate to 64-bit space
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
` (5 preceding siblings ...)
2013-06-21 10:46 ` [PATCH v4 6/8] hvmloader: Load large devices into high MMIO space as needed George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 11:22 ` Ian Jackson
2013-06-21 10:46 ` [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole George Dunlap
7 siblings, 1 reply; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
Allow devices with BARs less than 512MiB to be relocated to high
memory.
This will only be invoked if there is not enough low MMIO space to map
the device, and will be done preferentially to large devices first; so
in all likelihood only large devices will be remapped anyway.
This is needed to work-around the issue of qemu-xen not being able to
handle moving guest memory around to resize the MMIO hole. The
default MMIO hole size is less than 256MiB.
v3:
- Fixed minor style issue
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/config.h | 1 -
tools/firmware/hvmloader/pci.c | 5 ++---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 8143d6f..6641197 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -55,7 +55,6 @@ extern struct bios_config ovmf_config;
/* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
#define PCI_MEM_START 0xf0000000
#define PCI_MEM_END 0xfc000000
-#define PCI_MIN_BIG_BAR_SIZE 0x20000000
extern unsigned long pci_mem_start, pci_mem_end;
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 3f368f3..60e1a69 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -281,9 +281,8 @@ void pci_setup(void)
if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
PCI_BASE_ADDRESS_SPACE_MEMORY )
{
- /* Mapping high memory if PCI deivce is 64 bits bar and the bar size
- is larger than 512M */
- if (using_64bar && (bar_sz > PCI_MIN_BIG_BAR_SIZE)) {
+ /* Mapping high memory if PCI device is 64 bits bar */
+ if ( using_64bar ) {
if ( high_mem_resource.base & (bar_sz - 1) )
high_mem_resource.base = high_mem_resource.base -
(high_mem_resource.base & (bar_sz - 1)) + bar_sz;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 7/8] hvmloader: Remove minimum size for BARs to relocate to 64-bit space
2013-06-21 10:46 ` [PATCH v4 7/8] hvmloader: Remove minimum size for BARs to relocate to 64-bit space George Dunlap
@ 2013-06-21 11:22 ` Ian Jackson
0 siblings, 0 replies; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 11:22 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 7/8] hvmloader: Remove minimum size for BARs to relocate to 64-bit space"):
> Allow devices with BARs less than 512MiB to be relocated to high
> memory.
>
> This will only be invoked if there is not enough low MMIO space to map
> the device, and will be done preferentially to large devices first; so
> in all likelihood only large devices will be remapped anyway.
>
> This is needed to work-around the issue of qemu-xen not being able to
> handle moving guest memory around to resize the MMIO hole. The
> default MMIO hole size is less than 256MiB.
Assuming a good answer to my responses to 5/8 and 6/8, this is fine
and in line with what your 0/8 claims.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-21 10:46 [PATCH 0/8] Relocate devices rather than memory for qemu-xen George Dunlap
` (6 preceding siblings ...)
2013-06-21 10:46 ` [PATCH v4 7/8] hvmloader: Remove minimum size for BARs to relocate to 64-bit space George Dunlap
@ 2013-06-21 10:46 ` George Dunlap
2013-06-21 11:15 ` Stefano Stabellini
` (2 more replies)
7 siblings, 3 replies; 29+ messages in thread
From: George Dunlap @ 2013-06-21 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, Ian Jackson
At the moment, qemu-xen can't handle memory being relocated by
hvmloader. This may happen if a device with a large enough memory
region is passed through to the guest. At the moment, if this
happens, then at some point in the future qemu will crash and the
domain will hang. (qemu-traditional is fine.)
It's too late in the release to do a proper fix, so we try to do
damage control.
hvmloader already has mechanisms to relocate memory to 64-bit space if
it can't make a big enough MMIO hole. By default this is 2GiB; if we
just refuse to make the hole bigger if it will overlap with guest
memory, then the relocation will happen by default.
v4:
- Wrap long line in libxl_dm.c
- Fix comment
v3:
- Fix polarity of comparison
- Move diagnostic messages to another patch
- Tested with xen platform pci device hacked to have different BAR sizes
{256MiB, 1GiB} x {qemu-xen, qemu-traditional} x various memory
configurations
- Add comment explaining why we default to "allow"
- Remove cast to bool
v2:
- style fixes
- fix and expand comment on the MMIO hole loop
- use "%d" rather than "%s" -> (...)?"1":"0"
- use bool instead of uint8_t
- Move 64-bit bar relocate detection to another patch
- Add more diagnostic messages
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
CC: Keir Fraser <keir@xen.org>
---
tools/firmware/hvmloader/pci.c | 49 +++++++++++++++++++++++++++++--
tools/libxl/libxl_dm.c | 8 +++++
xen/include/public/hvm/hvm_xs_strings.h | 1 +
3 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 60e1a69..48edd5e 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -27,6 +27,8 @@
#include <xen/memory.h>
#include <xen/hvm/ioreq.h>
+#include <xen/hvm/hvm_xs_strings.h>
+#include <stdbool.h>
unsigned long pci_mem_start = PCI_MEM_START;
unsigned long pci_mem_end = PCI_MEM_END;
@@ -57,6 +59,32 @@ void pci_setup(void)
} *bars = (struct bars *)scratch_start;
unsigned int i, nr_bars = 0;
+ const char *s;
+ /*
+ * Do we allow hvmloader to relocate guest memory in order to
+ * increase the size of the lowmem MMIO hole? Defaulting to 1
+ * here will mean that non-libxl toolstacks (including xend and
+ * home-grown ones) will experience this series as "no change".
+ * It does mean that those using qemu-xen will still experience
+ * the bug (described below); but it also means that those using
+ * qemu-traditional will *not* experience any change; and it also
+ * means that there is a work-around for those using qemu-xen,
+ * namely switching to qemu-traditional.
+ *
+ * If we defaulted to 0, and failing to resize the hole caused any
+ * problems with qemu-traditional, then there is no work-around.
+ *
+ * Since xend can only use qemu-traditional, I think this is the
+ * option that will have the least impact.
+ */
+ bool allow_memory_relocate = 1;
+
+ s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
+ if ( s )
+ allow_memory_relocate = strtoll(s, NULL, 0);
+ printf("Relocating guest memory for lowmem MMIO space %s\n",
+ allow_memory_relocate?"enabled":"disabled");
+
/* Program PCI-ISA bridge with appropriate link routes. */
isa_irq = 0;
for ( link = 0; link < 4; link++ )
@@ -208,8 +236,25 @@ void pci_setup(void)
pci_writew(devfn, PCI_COMMAND, cmd);
}
- while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
- ((pci_mem_start << 1) != 0) )
+ /*
+ * At the moment qemu-xen can't deal with relocated memory regions.
+ * It's too close to the release to make a proper fix; for now,
+ * only allow the MMIO hole to grow large enough to move guest memory
+ * if we're running qemu-traditional. Items that don't fit will be
+ * relocated into the 64-bit address space.
+ *
+ * This loop now does the following:
+ * - If allow_memory_relocate, increase the MMIO hole until it's
+ * big enough, or until it's 2GiB
+ * - If !allow_memory_relocate, increase the MMIO hole until it's
+ * big enough, or until it's 2GiB, or until it overlaps guest
+ * memory
+ */
+ while ( (mmio_total > (pci_mem_end - pci_mem_start))
+ && ((pci_mem_start << 1) != 0)
+ && (allow_memory_relocate
+ || (((pci_mem_start << 1) >> PAGE_SHIFT)
+ >= hvm_info->low_mem_pgend)) )
pci_mem_start <<= 1;
if ( mmio_total > (pci_mem_end - pci_mem_start) )
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ac1f90e..7e54c02 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1154,6 +1154,14 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
libxl__xs_write(gc, XBT_NULL,
libxl__sprintf(gc, "%s/hvmloader/bios", path),
"%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
+ /* Disable relocating memory to make the MMIO hole larger
+ * unless we're running qemu-traditional */
+ libxl__xs_write(gc, XBT_NULL,
+ libxl__sprintf(gc,
+ "%s/hvmloader/allow-memory-relocate",
+ path),
+ "%d",
+ b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL);
free(path);
}
diff --git a/xen/include/public/hvm/hvm_xs_strings.h b/xen/include/public/hvm/hvm_xs_strings.h
index 9042303..4de5881 100644
--- a/xen/include/public/hvm/hvm_xs_strings.h
+++ b/xen/include/public/hvm/hvm_xs_strings.h
@@ -28,6 +28,7 @@
#define HVM_XS_HVMLOADER "hvmloader"
#define HVM_XS_BIOS "hvmloader/bios"
#define HVM_XS_GENERATION_ID_ADDRESS "hvmloader/generation-id-address"
+#define HVM_XS_ALLOW_MEMORY_RELOCATE "hvmloader/allow-memory-relocate"
/* The following values allow additional ACPI tables to be added to the
* virtual ACPI BIOS that hvmloader constructs. The values specify the guest
--
1.7.9.5
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-21 10:46 ` [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole George Dunlap
@ 2013-06-21 11:15 ` Stefano Stabellini
2013-06-21 11:25 ` Ian Jackson
2013-06-26 10:08 ` Hao, Xudong
2 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2013-06-21 11:15 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Ian Campbell, Hanweidong, xen-devel,
Stefano Stabellini, Ian Jackson
On Fri, 21 Jun 2013, George Dunlap wrote:
> At the moment, qemu-xen can't handle memory being relocated by
> hvmloader. This may happen if a device with a large enough memory
> region is passed through to the guest. At the moment, if this
> happens, then at some point in the future qemu will crash and the
> domain will hang. (qemu-traditional is fine.)
>
> It's too late in the release to do a proper fix, so we try to do
> damage control.
>
> hvmloader already has mechanisms to relocate memory to 64-bit space if
> it can't make a big enough MMIO hole. By default this is 2GiB; if we
> just refuse to make the hole bigger if it will overlap with guest
> memory, then the relocation will happen by default.
>
> v4:
> - Wrap long line in libxl_dm.c
> - Fix comment
> v3:
> - Fix polarity of comparison
> - Move diagnostic messages to another patch
> - Tested with xen platform pci device hacked to have different BAR sizes
> {256MiB, 1GiB} x {qemu-xen, qemu-traditional} x various memory
> configurations
> - Add comment explaining why we default to "allow"
> - Remove cast to bool
> v2:
> - style fixes
> - fix and expand comment on the MMIO hole loop
> - use "%d" rather than "%s" -> (...)?"1":"0"
> - use bool instead of uint8_t
> - Move 64-bit bar relocate detection to another patch
> - Add more diagnostic messages
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Hanweidong <hanweidong@huawei.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Keir Fraser <keir@xen.org>
> ---
> tools/firmware/hvmloader/pci.c | 49 +++++++++++++++++++++++++++++--
> tools/libxl/libxl_dm.c | 8 +++++
> xen/include/public/hvm/hvm_xs_strings.h | 1 +
> 3 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 60e1a69..48edd5e 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -27,6 +27,8 @@
>
> #include <xen/memory.h>
> #include <xen/hvm/ioreq.h>
> +#include <xen/hvm/hvm_xs_strings.h>
> +#include <stdbool.h>
>
> unsigned long pci_mem_start = PCI_MEM_START;
> unsigned long pci_mem_end = PCI_MEM_END;
> @@ -57,6 +59,32 @@ void pci_setup(void)
> } *bars = (struct bars *)scratch_start;
> unsigned int i, nr_bars = 0;
>
> + const char *s;
> + /*
> + * Do we allow hvmloader to relocate guest memory in order to
> + * increase the size of the lowmem MMIO hole? Defaulting to 1
> + * here will mean that non-libxl toolstacks (including xend and
> + * home-grown ones) will experience this series as "no change".
Sorry for being anal, but "this series" is also meaningless in a comment
within the code. What series? Maybe you could use "this commit" instead.
> + * It does mean that those using qemu-xen will still experience
> + * the bug (described below); but it also means that those using
> + * qemu-traditional will *not* experience any change; and it also
> + * means that there is a work-around for those using qemu-xen,
> + * namely switching to qemu-traditional.
> + *
> + * If we defaulted to 0, and failing to resize the hole caused any
> + * problems with qemu-traditional, then there is no work-around.
> + *
> + * Since xend can only use qemu-traditional, I think this is the
> + * option that will have the least impact.
> + */
> + bool allow_memory_relocate = 1;
> +
> + s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> + if ( s )
> + allow_memory_relocate = strtoll(s, NULL, 0);
> + printf("Relocating guest memory for lowmem MMIO space %s\n",
> + allow_memory_relocate?"enabled":"disabled");
> +
> /* Program PCI-ISA bridge with appropriate link routes. */
> isa_irq = 0;
> for ( link = 0; link < 4; link++ )
> @@ -208,8 +236,25 @@ void pci_setup(void)
> pci_writew(devfn, PCI_COMMAND, cmd);
> }
>
> - while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
> - ((pci_mem_start << 1) != 0) )
> + /*
> + * At the moment qemu-xen can't deal with relocated memory regions.
> + * It's too close to the release to make a proper fix; for now,
> + * only allow the MMIO hole to grow large enough to move guest memory
> + * if we're running qemu-traditional. Items that don't fit will be
> + * relocated into the 64-bit address space.
> + *
> + * This loop now does the following:
> + * - If allow_memory_relocate, increase the MMIO hole until it's
> + * big enough, or until it's 2GiB
> + * - If !allow_memory_relocate, increase the MMIO hole until it's
> + * big enough, or until it's 2GiB, or until it overlaps guest
> + * memory
> + */
> + while ( (mmio_total > (pci_mem_end - pci_mem_start))
> + && ((pci_mem_start << 1) != 0)
> + && (allow_memory_relocate
> + || (((pci_mem_start << 1) >> PAGE_SHIFT)
> + >= hvm_info->low_mem_pgend)) )
> pci_mem_start <<= 1;
>
> if ( mmio_total > (pci_mem_end - pci_mem_start) )
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..7e54c02 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1154,6 +1154,14 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
> libxl__xs_write(gc, XBT_NULL,
> libxl__sprintf(gc, "%s/hvmloader/bios", path),
> "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
> + /* Disable relocating memory to make the MMIO hole larger
> + * unless we're running qemu-traditional */
> + libxl__xs_write(gc, XBT_NULL,
> + libxl__sprintf(gc,
> + "%s/hvmloader/allow-memory-relocate",
> + path),
> + "%d",
> + b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL);
> free(path);
> }
>
> diff --git a/xen/include/public/hvm/hvm_xs_strings.h b/xen/include/public/hvm/hvm_xs_strings.h
> index 9042303..4de5881 100644
> --- a/xen/include/public/hvm/hvm_xs_strings.h
> +++ b/xen/include/public/hvm/hvm_xs_strings.h
> @@ -28,6 +28,7 @@
> #define HVM_XS_HVMLOADER "hvmloader"
> #define HVM_XS_BIOS "hvmloader/bios"
> #define HVM_XS_GENERATION_ID_ADDRESS "hvmloader/generation-id-address"
> +#define HVM_XS_ALLOW_MEMORY_RELOCATE "hvmloader/allow-memory-relocate"
>
> /* The following values allow additional ACPI tables to be added to the
> * virtual ACPI BIOS that hvmloader constructs. The values specify the guest
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-21 10:46 ` [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole George Dunlap
2013-06-21 11:15 ` Stefano Stabellini
@ 2013-06-21 11:25 ` Ian Jackson
2013-06-26 10:08 ` Hao, Xudong
2 siblings, 0 replies; 29+ messages in thread
From: Ian Jackson @ 2013-06-21 11:25 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Hanweidong, Stefano Stabellini, Ian Campbell, xen-devel
George Dunlap writes ("[PATCH v4 8/8] libxl,hvmloader: Don't relocate memory for MMIO hole"):
> At the moment, qemu-xen can't handle memory being relocated by
> hvmloader. This may happen if a device with a large enough memory
> region is passed through to the guest. At the moment, if this
> happens, then at some point in the future qemu will crash and the
> domain will hang. (qemu-traditional is fine.)
>
> It's too late in the release to do a proper fix, so we try to do
> damage control.
>
> hvmloader already has mechanisms to relocate memory to 64-bit space if
> it can't make a big enough MMIO hole. By default this is 2GiB; if we
> just refuse to make the hole bigger if it will overlap with guest
> memory, then the relocation will happen by default.
I see you still haven't changed it to use GCSPRINTF but I don't think
that's worth arguing about right now.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-21 10:46 ` [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole George Dunlap
2013-06-21 11:15 ` Stefano Stabellini
2013-06-21 11:25 ` Ian Jackson
@ 2013-06-26 10:08 ` Hao, Xudong
2013-06-26 13:36 ` Stefano Stabellini
2 siblings, 1 reply; 29+ messages in thread
From: Hao, Xudong @ 2013-06-26 10:08 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: Stefano Stabellini, Ian Jackson, Keir Fraser, Ian Campbell, Hanweidong
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of George Dunlap
> Sent: Friday, June 21, 2013 6:47 PM
> To: xen-devel@lists.xen.org
> Cc: Keir Fraser; Ian Campbell; Hanweidong; George Dunlap; Stefano Stabellini;
> Ian Jackson
> Subject: [Xen-devel] [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for
> MMIO hole
>
> At the moment, qemu-xen can't handle memory being relocated by
> hvmloader. This may happen if a device with a large enough memory
> region is passed through to the guest. At the moment, if this
> happens, then at some point in the future qemu will crash and the
> domain will hang. (qemu-traditional is fine.)
>
> It's too late in the release to do a proper fix, so we try to do
> damage control.
>
> hvmloader already has mechanisms to relocate memory to 64-bit space if
> it can't make a big enough MMIO hole. By default this is 2GiB; if we
> just refuse to make the hole bigger if it will overlap with guest
> memory, then the relocation will happen by default.
>
For qemu-xen use case, hvmloader start MMIO hole at 0xf0000000. However, qemu-xen initialize ram region to 0xf0000000(HVM_BELOW_4G_RAM_END) below 4G, and pci hole starting from 0xe0000000, is it overlap?
-thanks
Xudong
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-26 10:08 ` Hao, Xudong
@ 2013-06-26 13:36 ` Stefano Stabellini
2013-06-26 14:23 ` Hao, Xudong
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2013-06-26 13:36 UTC (permalink / raw)
To: Hao, Xudong
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap, xen-devel,
Stefano Stabellini, Ian Jackson
On Wed, 26 Jun 2013, Hao, Xudong wrote:
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org
> > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of George Dunlap
> > Sent: Friday, June 21, 2013 6:47 PM
> > To: xen-devel@lists.xen.org
> > Cc: Keir Fraser; Ian Campbell; Hanweidong; George Dunlap; Stefano Stabellini;
> > Ian Jackson
> > Subject: [Xen-devel] [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for
> > MMIO hole
> >
> > At the moment, qemu-xen can't handle memory being relocated by
> > hvmloader. This may happen if a device with a large enough memory
> > region is passed through to the guest. At the moment, if this
> > happens, then at some point in the future qemu will crash and the
> > domain will hang. (qemu-traditional is fine.)
> >
> > It's too late in the release to do a proper fix, so we try to do
> > damage control.
> >
> > hvmloader already has mechanisms to relocate memory to 64-bit space if
> > it can't make a big enough MMIO hole. By default this is 2GiB; if we
> > just refuse to make the hole bigger if it will overlap with guest
> > memory, then the relocation will happen by default.
> >
>
> For qemu-xen use case, hvmloader start MMIO hole at 0xf0000000. However, qemu-xen initialize ram region to 0xf0000000(HVM_BELOW_4G_RAM_END) below 4G, and pci hole starting from 0xe0000000, is it overlap?
hvmloader configures the MMIO hole to start at 0xf0000000, qemu-xen
configures the below_4g_mem_size ram region to *end* at 0xf0000000 and
the pci hole to start from 0xf0000000. It's all coherent now.
The patch to modify the pci hole in qemu-xen and have it start at
0xe0000000 has been reverted.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-26 13:36 ` Stefano Stabellini
@ 2013-06-26 14:23 ` Hao, Xudong
2013-06-26 16:21 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Hao, Xudong @ 2013-06-26 14:23 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap, xen-devel,
Stefano Stabellini, Ian Jackson
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Wednesday, June 26, 2013 9:36 PM
> To: Hao, Xudong
> Cc: George Dunlap; xen-devel@lists.xen.org; Keir Fraser; Ian Campbell;
> Hanweidong; Stefano Stabellini; Ian Jackson
> Subject: RE: [Xen-devel] [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory
> for MMIO hole
>
> On Wed, 26 Jun 2013, Hao, Xudong wrote:
> > > -----Original Message-----
> > > From: xen-devel-bounces@lists.xen.org
> > > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of George Dunlap
> > > Sent: Friday, June 21, 2013 6:47 PM
> > > To: xen-devel@lists.xen.org
> > > Cc: Keir Fraser; Ian Campbell; Hanweidong; George Dunlap; Stefano
> Stabellini;
> > > Ian Jackson
> > > Subject: [Xen-devel] [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory
> for
> > > MMIO hole
> > >
> > > At the moment, qemu-xen can't handle memory being relocated by
> > > hvmloader. This may happen if a device with a large enough memory
> > > region is passed through to the guest. At the moment, if this
> > > happens, then at some point in the future qemu will crash and the
> > > domain will hang. (qemu-traditional is fine.)
> > >
> > > It's too late in the release to do a proper fix, so we try to do
> > > damage control.
> > >
> > > hvmloader already has mechanisms to relocate memory to 64-bit space if
> > > it can't make a big enough MMIO hole. By default this is 2GiB; if we
> > > just refuse to make the hole bigger if it will overlap with guest
> > > memory, then the relocation will happen by default.
> > >
> >
> > For qemu-xen use case, hvmloader start MMIO hole at 0xf0000000. However,
> qemu-xen initialize ram region to 0xf0000000(HVM_BELOW_4G_RAM_END)
> below 4G, and pci hole starting from 0xe0000000, is it overlap?
>
> hvmloader configures the MMIO hole to start at 0xf0000000, qemu-xen
> configures the below_4g_mem_size ram region to *end* at 0xf0000000 and
That's right.
> the pci hole to start from 0xf0000000. It's all coherent now.
>
Current qemu upstream configure pci hole as below, do I miss something?
if (ram_size >= 0xe0000000 ) {
above_4g_mem_size = ram_size - 0xe0000000;
below_4g_mem_size = 0xe0000000;
} else {
above_4g_mem_size = 0;
below_4g_mem_size = ram_size;
}
Thanks,
-Xudong
> The patch to modify the pci hole in qemu-xen and have it start at
> 0xe0000000 has been reverted.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole
2013-06-26 14:23 ` Hao, Xudong
@ 2013-06-26 16:21 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2013-06-26 16:21 UTC (permalink / raw)
To: Hao, Xudong
Cc: Keir Fraser, Ian Campbell, Hanweidong, George Dunlap,
Stefano Stabellini, xen-devel, Stefano Stabellini, Ian Jackson
On Wed, 26 Jun 2013, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Wednesday, June 26, 2013 9:36 PM
> > To: Hao, Xudong
> > Cc: George Dunlap; xen-devel@lists.xen.org; Keir Fraser; Ian Campbell;
> > Hanweidong; Stefano Stabellini; Ian Jackson
> > Subject: RE: [Xen-devel] [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory
> > for MMIO hole
> >
> > On Wed, 26 Jun 2013, Hao, Xudong wrote:
> > > > -----Original Message-----
> > > > From: xen-devel-bounces@lists.xen.org
> > > > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of George Dunlap
> > > > Sent: Friday, June 21, 2013 6:47 PM
> > > > To: xen-devel@lists.xen.org
> > > > Cc: Keir Fraser; Ian Campbell; Hanweidong; George Dunlap; Stefano
> > Stabellini;
> > > > Ian Jackson
> > > > Subject: [Xen-devel] [PATCH v4 8/8] libxl, hvmloader: Don't relocate memory
> > for
> > > > MMIO hole
> > > >
> > > > At the moment, qemu-xen can't handle memory being relocated by
> > > > hvmloader. This may happen if a device with a large enough memory
> > > > region is passed through to the guest. At the moment, if this
> > > > happens, then at some point in the future qemu will crash and the
> > > > domain will hang. (qemu-traditional is fine.)
> > > >
> > > > It's too late in the release to do a proper fix, so we try to do
> > > > damage control.
> > > >
> > > > hvmloader already has mechanisms to relocate memory to 64-bit space if
> > > > it can't make a big enough MMIO hole. By default this is 2GiB; if we
> > > > just refuse to make the hole bigger if it will overlap with guest
> > > > memory, then the relocation will happen by default.
> > > >
> > >
> > > For qemu-xen use case, hvmloader start MMIO hole at 0xf0000000. However,
> > qemu-xen initialize ram region to 0xf0000000(HVM_BELOW_4G_RAM_END)
> > below 4G, and pci hole starting from 0xe0000000, is it overlap?
> >
> > hvmloader configures the MMIO hole to start at 0xf0000000, qemu-xen
> > configures the below_4g_mem_size ram region to *end* at 0xf0000000 and
>
> That's right.
>
> > the pci hole to start from 0xf0000000. It's all coherent now.
> >
>
> Current qemu upstream configure pci hole as below, do I miss something?
>
> if (ram_size >= 0xe0000000 ) {
> above_4g_mem_size = ram_size - 0xe0000000;
> below_4g_mem_size = 0xe0000000;
> } else {
> above_4g_mem_size = 0;
> below_4g_mem_size = ram_size;
> }
That's the non-Xen case.
Give a look at xen-all.c:xen_ram_init:
if (ram_size >= HVM_BELOW_4G_RAM_END) {
above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
below_4g_mem_size = HVM_BELOW_4G_RAM_END;
} else {
below_4g_mem_size = ram_size;
}
^ permalink raw reply [flat|nested] 29+ messages in thread