thr3ads.net - Xen devel - [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole [Jun 2013]

If this information is useful, please help other people find it:
Share via:

George Dunlap

2013-Jun-17 16:43 UTC

[PATCH] 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 this codepath will be taken by default.

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>
---
 tools/firmware/hvmloader/pci.c          |   24 +++++++++++++++++++++---
 tools/libxl/libxl_dm.c                  |    7 +++++++
 xen/include/public/hvm/hvm_xs_strings.h |    1 +
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index c78d4d3..9682a1f 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -27,6 +27,7 @@
 
 #include <xen/memory.h>
 #include <xen/hvm/ioreq.h>
+#include <xen/hvm/hvm_xs_strings.h>
 
 unsigned long pci_mem_start = PCI_MEM_START;
 unsigned long pci_mem_end = PCI_MEM_END;
@@ -58,6 +59,15 @@ void pci_setup(void)
     } *bars = (struct bars *)scratch_start;
     unsigned int i, nr_bars = 0;
 
+    const char *s;
+    uint8_t allow_memory_relocate=1;
+
+    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
+    if (s)
+        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
+    printf("allow_memory_relocate %u\n",
+           __func__, allow_memory_relocate);
+
     /* Program PCI-ISA bridge with appropriate link routes. */
     isa_irq = 0;
     for ( link = 0; link < 4; link++ )
@@ -209,11 +219,19 @@ 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,
+     * if we''re using qemu-xen, just crash and tell people to switch
+     * to qemu-traditional.
+     */
+    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 ( (pci_mem_start << 1) != 0 )
+    if (mmio_total > (pci_mem_end - pci_mem_start))
         bar64_relocate = 1;
 
     /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ac1f90e..5167ee0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1154,6 +1154,13 @@ 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),
+                        "%s",
+                       
(b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL)?
+                        "1":"0");
         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

George Dunlap

2013-Jun-17 16:48 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 17/06/13 17:43, 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 this codepath will be taken by default.
>
> 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>
Unfortunatley, I don''t have an easy way to test this, as all of the 
devices on my system are far smaller than 256MiB.

Weidong, could you give this patch a try and see if it works for you?

  -George

George Dunlap

2013-Jun-17 17:02 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 17/06/13 17:43, 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 this codepath will be taken by default.
>
> 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>
This isn''t exactly what we had discussed, but looking at the code, I 
think this is the easiest thing to do.

A brief description of what hvmloader does:
1. enumerate all BARs from all devices, ordering them from large to small
2. Double* the size of the <4G MMIO hole until it is big enough to fit 
all the MMIO, or until it reaches 2GiB, whichever comes first
3. Move guest memory which overlaps the <4G MMIO hole above 4G.
4. Place devices in memory, placing devices above 4G if the hole isn''t 
big enough for everything.

Unfortunately, the option of "just don''t map the passed-through
device
if there''s not enough MMIO space" doesn''t really work --
there''s no way
to really tell which device is the one you shouldn''t map.

Of course we have the same issue with relocating devices to 64-bit: we 
can''t tell which devices will be located above 4G or not.  So for 
devices not capable of accessing physical address space above 4G, random 
devices will simply not appear; this may cause strange problems if the 
emulated network card or disk, for example, is not visible.  There may 
also be examples of assumptions baked into drivers or emulation code 
about the physical address space.  On the whole, given that this is 
KVM''s behavior, I''m inclined to be optimistic about the state
of guest
OSes and drivers.

Ian Jackson had proposed just crashing in hvmloader if we were using 
qemu-xen and needed to relocate memory.  This changes a "crash randomly 
at some point after boot" into a "crash immediately before the BIOS
even
runs".  While that is certainly an improvement, I think on the whole 
that it''s still bad enough that "mostly works but occasionally has
strange failures" is still better; hence this patch.

  -George

Wei Liu

2013-Jun-17 17:09 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On Mon, Jun 17, 2013 at 05:43:47PM +0100, George Dunlap wrote:
[...]>  
>  #include <xen/memory.h>
>  #include <xen/hvm/ioreq.h>
> +#include <xen/hvm/hvm_xs_strings.h>
>  
>  unsigned long pci_mem_start = PCI_MEM_START;
>  unsigned long pci_mem_end = PCI_MEM_END;
> @@ -58,6 +59,15 @@ void pci_setup(void)
>      } *bars = (struct bars *)scratch_start;
>      unsigned int i, nr_bars = 0;
>  
> +    const char *s;
> +    uint8_t allow_memory_relocate=1;
Missing white space around ''=''. And there are similar style
problems
below as well.
> +
> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> +    if (s)
> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
> +    printf("allow_memory_relocate %u\n",
> +           __func__, allow_memory_relocate);
Missing "%s" in format string?


Wei.

Ian Jackson

2013-Jun-17 17:15 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

George Dunlap writes ("[PATCH] 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.)
I think the approach is good.  Arguably the two things should be in
two patches.
> +    const char *s;
> +    uint8_t allow_memory_relocate=1;
> +
> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> +    if (s)
> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
the cast.  (Also, spaces round `=''.)
> +    printf("allow_memory_relocate %u\n",
> +           __func__, allow_memory_relocate);
Missing %s argument.  This shows that -Wformat isn''t working.
> -    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,
> +     * if we''re using qemu-xen, just crash and tell people to
switch
> +     * to qemu-traditional.
> +     */
> +    while ((mmio_total > (pci_mem_end - pci_mem_start)) 
> +           && ((pci_mem_start << 1) != 0)
> +           && ( allow_memory_relocate
I think this is an unnecessarily confusing way of writing it.
allow_memory_relocate does not change.  I would lift it out of the
loop into an if.  That would also mean that "git-diff -b" can show
more clearly whether the change is truly what you intend.
> +                || (((pci_mem_start << 1)>>PAGE_SHIFT
)<hvm_info->low_mem_pgend))
>          pci_mem_start <<= 1;
>  
> -    if ( (pci_mem_start << 1) != 0 )
> +    if (mmio_total > (pci_mem_end - pci_mem_start))
>          bar64_relocate = 1;
Missing spaces.
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..5167ee0 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1154,6 +1154,13 @@ 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 */
This whole hunk could do with more aggressive wrapping.  See what it
looks like in my MUA.  Can you keep it down to 70-75 columns ?
> +        libxl__xs_write(gc, XBT_NULL,
> +                        libxl__sprintf(gc,
"%s/hvmloader/allow-memory-reloca
te", path),> +                        "%s",
> +                       
(b_info->device_model_version==LIBXL_DEVICE_MODEL_VE
RSION_QEMU_XEN_TRADITIONAL)?> +                        "1":"0");
>          free(path);
Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
value from libxl__xs_write).

Instead of "%s", blah?"1":"0" use "%d"
and the == expression itself.

Ian.

George Dunlap

2013-Jun-18 09:50 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 06/17/2013 06:15 PM, Ian Jackson wrote:> George Dunlap writes ("[PATCH] 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.)
>
> I think the approach is good.  Arguably the two things should be in
> two patches.
You mean one in libxl to set allow_memory_relocate, one to do something 
about it?
>
>> +    const char *s;
>> +    uint8_t allow_memory_relocate=1;
>> +
>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>> +    if (s)
>> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
>
> Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
> the cast.  (Also, spaces round `=''.)
Remember that hvmloader doesn''t actually have a libc; this is a locally
implemented function, and AFAICT the only one implemented is strtoll.
>
>> +    printf("allow_memory_relocate %u\n",
>> +           __func__, allow_memory_relocate);
>
> Missing %s argument.  This shows that -Wformat isn''t working.
No, my bad -- I had a script which I thought was compiling all the 
tools, but in fact was *only* compiling libxl/xl.  -Wformat is indeed 
working.
>
>> -    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,
>> +     * if we''re using qemu-xen, just crash and tell people to
switch
>> +     * to qemu-traditional.
>> +     */
>> +    while ((mmio_total > (pci_mem_end - pci_mem_start))
>> +           && ((pci_mem_start << 1) != 0)
>> +           && ( allow_memory_relocate
>
> I think this is an unnecessarily confusing way of writing it.
> allow_memory_relocate does not change.  I would lift it out of the
> loop into an if.  That would also mean that "git-diff -b" can
show
> more clearly whether the change is truly what you intend.
But the behavior we want is this:
* If allow_memory_relocate, increase the size of the memory hole until 
it''s big enough, or it reaches 2GiB
* If !allow_memory_relocate, increase the size of the memory hole until 
it''s big enough, or it reaches 2GiB, or it overlaps guest memory.

If we pulled "allow_memory_relocate" out, we''d have to make
two separate
while() loops.  That might make it more clear what was going on, but 
would risk introducing errors between the two copies for the other 
conditions.
>
>> +                || (((pci_mem_start << 1)>>PAGE_SHIFT
)<hvm_info->low_mem_pgend))
>>           pci_mem_start <<= 1;
>>
>> -    if ( (pci_mem_start << 1) != 0 )
>> +    if (mmio_total > (pci_mem_end - pci_mem_start))
>>           bar64_relocate = 1;
>
> Missing spaces.
>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index ac1f90e..5167ee0 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -1154,6 +1154,13 @@ 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 */
>
> This whole hunk could do with more aggressive wrapping.  See what it
> looks like in my MUA.  Can you keep it down to 70-75 columns ?
Sure.
>
>> +        libxl__xs_write(gc, XBT_NULL,
>> +                        libxl__sprintf(gc,
"%s/hvmloader/allow-memory-reloca
> te", path),
>> +                        "%s",
>> +                       
(b_info->device_model_version==LIBXL_DEVICE_MODEL_VE
> RSION_QEMU_XEN_TRADITIONAL)?
>> +                        "1":"0");
>>           free(path);
>
> Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
> value from libxl__xs_write).
Once again, I disagree with mixing different code styles in the same 
function.  Using GCSPRINTF and doing an error check here will make it 
seem like it''s doing something different than the line immediately
above
it (which is what I copied to get here), which will confuse people.

In theory the "fix it up as we go along" sounds good, but in practice 
it''s awful.  How many times have you had to say exactly this kind of 
thing, when all I did was copy some bit of code that was nearby and do 
exactly the same thing?  It just annoys you and frustrates me.

I shouldn''t have to keep in my head, "This is the new way of doing
things".  It''s a total waste of mental energy; I have better
things to
spend my brain capacity on.  If you want things to change, I think
we''re
going to have to have a "flag day" where we go through and change all 
the code, at least file-by-file.  I''d be willing to help with this 
cleanup once the 4.4 development window opens.
> Instead of "%s", blah?"1":"0" use
"%d" and the == expression itself.
That''s a good idea.

  -George

George Dunlap

2013-Jun-18 09:57 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 06/18/2013 10:50 AM, George Dunlap wrote:> On 06/17/2013 06:15 PM, Ian Jackson wrote:
>> George Dunlap writes ("[PATCH] 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.)
>>
>> I think the approach is good.  Arguably the two things should be in
>> two patches.
>
> You mean one in libxl to set allow_memory_relocate, one to do something
> about it?
>
>>
>>> +    const char *s;
>>> +    uint8_t allow_memory_relocate=1;
>>> +
>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>>> +    if (s)
>>> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
>>
>> Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
>> the cast.  (Also, spaces round `=''.)
>
> Remember that hvmloader doesn''t actually have a libc; this is a
locally
> implemented function, and AFAICT the only one implemented is strtoll.
Hmm, and it doesn''t seem to have a bool_t either:

pci.c: In function ‘pci_setup’:
pci.c:63:5: error: unknown type name ‘bool_t’
make[7]: *** [pci.o] Error 1

uint8_t is what this uses for various other booleans (is_64bar, &c), so 
I''ll just follow suit.

  -G

Ian Jackson

2013-Jun-18 10:53 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

George Dunlap writes ("Re: [PATCH] libxl,hvmloader: Don''t relocate
memory for MMIO hole"):> On 06/17/2013 06:15 PM, Ian Jackson wrote:
> > I think the approach is good.  Arguably the two things should be in
> > two patches.
> 
> You mean one in libxl to set allow_memory_relocate, one to do something 
> about it?
No.  I mean one to do the thing with allow_memory_relocate and one to
change the condition on bar64_relocate.  But maybe I have
misunderstood and that doesn''t make sense.  Anyway, it''s OK
IMO as one
patch.
> >> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> >> +    if (s)
> >> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
> >
> > Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
> > the cast.  (Also, spaces round `=''.)
> 
> Remember that hvmloader doesn''t actually have a libc; this is a
locally
> implemented function, and AFAICT the only one implemented is strtoll.
_Bool is a compiler feature and available without the use of
stdbool.h.  It has better semantics than uintblah_t.

It seems curious that one would implement strtoll but not strtoull.
The latter is easier.  Oh well.  strtoll will do, but you should
convert the result to a _Bool.
> > I think this is an unnecessarily confusing way of writing it.
> > allow_memory_relocate does not change.  I would lift it out of the
> > loop into an if.  That would also mean that "git-diff -b"
can show
> > more clearly whether the change is truly what you intend.
> 
> But the behavior we want is this:
Sorry, I misread the loop.  You are right.
> > Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
> > value from libxl__xs_write).
> 
> Once again, I disagree with mixing different code styles in the same 
> function.  Using GCSPRINTF and doing an error check here will make it 
> seem like it''s doing something different than the line immediately
above
> it (which is what I copied to get here), which will confuse people.
Perhaps we should sweep through libxl sorting out these kind of style
things in 4.4.  Clearly now isn''t the time for this kind of wholesale
change.

In the meantime I think we do want to avoid introducing new code with
deprecated style.

Thanks,
Ian.

George Dunlap

2013-Jun-18 11:43 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 06/18/2013 11:53 AM, Ian Jackson wrote:> George Dunlap writes ("Re: [PATCH] libxl,hvmloader: Don''t
relocate memory for MMIO hole"):
>> On 06/17/2013 06:15 PM, Ian Jackson wrote:
>>> I think the approach is good.  Arguably the two things should be in
>>> two patches.
>>
>> You mean one in libxl to set allow_memory_relocate, one to do something
>> about it?
>
> No.  I mean one to do the thing with allow_memory_relocate and one to
> change the condition on bar64_relocate.  But maybe I have
> misunderstood and that doesn''t make sense.  Anyway, it''s
OK IMO as one
> patch.
Yes, I was thinking that if we were breaking down the patches, that 
might be a candidate for a separate patch.  Why don''t I do that anyway.
>
>>>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>>>> +    if (s)
>>>> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
>>>
>>> Use strtoul, surely, and bool_t (or _Bool).  Then there is no need
for
>>> the cast.  (Also, spaces round `=''.)
>>
>> Remember that hvmloader doesn''t actually have a libc; this is
a locally
>> implemented function, and AFAICT the only one implemented is strtoll.
>
> _Bool is a compiler feature and available without the use of
> stdbool.h.  It has better semantics than uintblah_t.
We apparently have stdbool.h, but it defines "bool" instead of
"bool_t". :-)
>>> Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
>>> value from libxl__xs_write).
>>
>> Once again, I disagree with mixing different code styles in the same
>> function.  Using GCSPRINTF and doing an error check here will make it
>> seem like it''s doing something different than the line
immediately above
>> it (which is what I copied to get here), which will confuse people.
>
> Perhaps we should sweep through libxl sorting out these kind of style
> things in 4.4.  Clearly now isn''t the time for this kind of
wholesale
> change.
>
> In the meantime I think we do want to avoid introducing new code with
> deprecated style.
I''m afraid if that''s what you want you''re going to
have to do it
yourself.  I think mixing code style is much worse than having a 
consistent, but deprecated code style, and I''m not going to do it.

  -George

Hanweidong

2013-Jun-18 12:18 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
> Sent: 2013年6月18日 0:49
> To: George Dunlap
> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
> Stabellini; Hanweidong
> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory for MMIO
> hole
> 
> On 17/06/13 17:43, 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 this codepath will be taken by default.
> >
> > 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>
> 
> Unfortunatley, I don't have an easy way to test this, as all of the
> devices on my system are far smaller than 256MiB.
> 
> Weidong, could you give this patch a try and see if it works for you?
> 
George，

I don't have a system in hand to test it today. I try to have a test
tomorrow. In my experience, when it's prevented to relocate memory for MMIO
hole by your patch, windows guest will crash (blue screen) and linux guest will
hang with a blank display.

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

George Dunlap

2013-Jun-18 12:20 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 06/18/2013 01:18 PM, Hanweidong wrote:>> -----Original Message-----
>> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
>> Sent: 2013年6月18日 0:49
>> To: George Dunlap
>> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
>> Stabellini; Hanweidong
>> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory for
MMIO
>> hole
>>
>> On 17/06/13 17:43, 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 this codepath will be taken by default.
>>>
>>> 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>
>>
>> Unfortunatley, I don't have an easy way to test this, as all of the
>> devices on my system are far smaller than 256MiB.
>>
>> Weidong, could you give this patch a try and see if it works for you?
>>
>
> George，
>
> I don't have a system in hand to test it today. I try to have a test
tomorrow. In my experience, when it's prevented to relocate memory for MMIO
hole by your patch, windows guest will crash (blue screen) and linux guest will
hang with a blank display.
You mean to say, you've tried this approach before (relocating to a 
64-bit MMIO region) and it doesn't work?

BTW, don't try this patch -- it probably won't compile.  I've got a
v2
coming up to use instead.

  -George

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

George Dunlap

2013-Jun-18 12:44 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 06/18/2013 01:18 PM, Hanweidong wrote:>> -----Original Message-----
>> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
>> Sent: 2013年6月18日 0:49
>> To: George Dunlap
>> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
>> Stabellini; Hanweidong
>> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory for
MMIO
>> hole
>>
>> On 17/06/13 17:43, 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 this codepath will be taken by default.
>>>
>>> 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>
>>
>> Unfortunatley, I don't have an easy way to test this, as all of the
>> devices on my system are far smaller than 256MiB.
>>
>> Weidong, could you give this patch a try and see if it works for you?
>>
>
> George，
>
> I don't have a system in hand to test it today. I try to have a test
tomorrow. In my experience, when it's prevented to relocate memory for MMIO
hole by your patch, windows guest will crash (blue screen) and linux guest will
hang with a blank display.
Actually, did you have a way of testing this without actually having a 
real device to test it with?  That would help me sort things out much 
more quickly.

  -George

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

Hanweidong

2013-Jun-18 12:46 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
> Sent: 2013年6月18日 20:21
> To: Hanweidong
> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
> Stabellini
> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory for MMIO
> hole
> 
> On 06/18/2013 01:18 PM, Hanweidong wrote:
> >> -----Original Message-----
> >> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
> >> Sent: 2013年6月18日 0:49
> >> To: George Dunlap
> >> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
> >> Stabellini; Hanweidong
> >> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory
for MMIO
> >> hole
> >>
> >> On 17/06/13 17:43, 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 this codepath will be taken by default.
> >>>
> >>> 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>
> >>
> >> Unfortunatley, I don't have an easy way to test this, as all
of the
> >> devices on my system are far smaller than 256MiB.
> >>
> >> Weidong, could you give this patch a try and see if it works for
you?
> >>
> >
> > George，
> >
> > I don't have a system in hand to test it today. I try to have a
test
> tomorrow. In my experience, when it's prevented to relocate memory for
> MMIO hole by your patch, windows guest will crash (blue screen) and
> linux guest will hang with a blank display.
> 
> You mean to say, you've tried this approach before (relocating to a
> 64-bit MMIO region) and it doesn't work?
I didn't try this approach. I missed you want to map mmio to 64-bit space
instead of relocate memory. Currently only mmio size is larger than 512M
(PCI_MIN_BIG_BAR_SIZE) will be map to 64-bit space. So you need to change the
condition if you want to map smaller mmio to 64-bit space.

weidong


> 
> BTW, don't try this patch -- it probably won't compile.  I've
got a v2
> coming up to use instead.
> 
>   -George_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

George Dunlap

2013-Jun-18 12:51 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

On 06/18/2013 01:46 PM, Hanweidong wrote:>
>
>> -----Original Message-----
>> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
>> Sent: 2013年6月18日 20:21
>> To: Hanweidong
>> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
>> Stabellini
>> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory for
MMIO
>> hole
>>
>> On 06/18/2013 01:18 PM, Hanweidong wrote:
>>>> -----Original Message-----
>>>> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
>>>> Sent: 2013年6月18日 0:49
>>>> To: George Dunlap
>>>> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
>>>> Stabellini; Hanweidong
>>>> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory
for MMIO
>>>> hole
>>>>
>>>> On 17/06/13 17:43, 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 this codepath will be taken by default.
>>>>>
>>>>> 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>
>>>>
>>>> Unfortunatley, I don't have an easy way to test this, as
all of the
>>>> devices on my system are far smaller than 256MiB.
>>>>
>>>> Weidong, could you give this patch a try and see if it works
for you?
>>>>
>>>
>>> George，
>>>
>>> I don't have a system in hand to test it today. I try to have a
test
>> tomorrow. In my experience, when it's prevented to relocate memory
for
>> MMIO hole by your patch, windows guest will crash (blue screen) and
>> linux guest will hang with a blank display.
>>
>> You mean to say, you've tried this approach before (relocating to a
>> 64-bit MMIO region) and it doesn't work?
>
> I didn't try this approach. I missed you want to map mmio to 64-bit
space instead of relocate memory. Currently only mmio size is larger than 512M
(PCI_MIN_BIG_BAR_SIZE) will be map to 64-bit space. So you need to change the
condition if you want to map smaller mmio to 64-bit space.
Ah, blast -- I forgot about that part.

Looks like the change will have to be bigger... :-(

  -George


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

Hanweidong

2013-Jun-18 12:54 UTC

head link

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
> Sent: 2013年6月18日 20:44
> To: Hanweidong
> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
> Stabellini
> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory for MMIO
> hole
> 
> On 06/18/2013 01:18 PM, Hanweidong wrote:
> >> -----Original Message-----
> >> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
> >> Sent: 2013年6月18日 0:49
> >> To: George Dunlap
> >> Cc: xen-devel@lists.xen.org; Ian Campbell; Ian Jackson; Stefano
> >> Stabellini; Hanweidong
> >> Subject: Re: [PATCH] libxl,hvmloader: Don't relocate memory
for MMIO
> >> hole
> >>
> >> On 17/06/13 17:43, 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 this codepath will be taken by default.
> >>>
> >>> 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>
> >>
> >> Unfortunatley, I don't have an easy way to test this, as all
of the
> >> devices on my system are far smaller than 256MiB.
> >>
> >> Weidong, could you give this patch a try and see if it works for
you?
> >>
> >
> > George，
> >
> > I don't have a system in hand to test it today. I try to have a
test
> tomorrow. In my experience, when it's prevented to relocate memory for
> MMIO hole by your patch, windows guest will crash (blue screen) and
> linux guest will hang with a blank display.
> 
> Actually, did you have a way of testing this without actually having a
> real device to test it with?  That would help me sort things out much
> more quickly.
> 
I think you can try it with ivshmem device emulated in QEMU. Ivshmem device can
be configured with large mmio. Ivshmem code only be compiled for kvm by default,
you can change the makefile to compile it for xen also. And then hardcode to
create a ivshmem device for a VM in vl.c. At last, you also need to run the
ivshmem-server in dom0.

weidong

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

Xen devel - Jun 2013 - [PATCH] libxl, hvmloader: Don't relocate memory for MMIO hole

[PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole

Re: [PATCH] libxl, hvmloader: Don''t relocate memory for MMIO hole