Re: HVM support for e820_host (Was: Bug: Limitation of <=2GB RAM in domU persists with 4.3.0)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Sep 04, 2013 at 01:18:39AM +0100, Gordan Bobic wrote:
> On 09/03/2013 10:30 PM, Konrad Rzeszutek Wilk wrote:
> >On Tue, Sep 03, 2013 at 10:24:44PM +0100, Gordan Bobic wrote:
> >>On 09/03/2013 10:10 PM, Konrad Rzeszutek Wilk wrote:
> >>>On Tue, Sep 03, 2013 at 09:49:40PM +0100, Gordan Bobic wrote:
> >>>>I spoke too soon - even with e820_host=0, the same error occurs.
> >>>>What did I break? The code in question is this:
> >>>>
> >>>>if (libxl_defbool_val(d_config->b_info.e820_host)) {
> >>>>     ret = libxl__e820_alloc(gc, domid, d_config);
> >>>>     if (ret) {
> >>>>         LIBXL__LOG_ERRNO(gc->owner, LIBXL__LOG_ERROR,
> >>>>                 "Failed while collecting E820 with: %d (errno:%d)\n",
> >>>>                 ret, errno);
> >>>>     }
> >>>>}
> >>>>
> >>>>With e820_host=0, that outer black should evaluate to false, should
> >>>>it not? In libxl_create.c, if I am understanding the code correctly,
> >>>>e820_host is defaulted to false, too. What am I missing?
> >
> >Does your config have 'pci' in it? The patch you sent had this:
> >
> >+        if (d_config->num_pcidevs)
> >+            libxl_defbool_set(&b_info->e820_host, true);
> >
> >Which means that even if you did not have e820_host it will be automatically
> >set if you have PCI devices.
> 
> OK - that was embarrasing. Caffeine underflow error. :(
> I backed out that block. I don't think e820_host should be implicit
> in hvm when PCI devices are passed.
> 
> That makes the adjusted patch fragment:
> --- xl_cmdimpl.c.orig	2013-09-04 00:42:57.424337503 +0100
> +++ xl_cmdimpl.c	2013-09-04 00:43:21.213886356 +0100
> @@ -1293,7 +1293,7 @@
>                  d_config->num_pcidevs++;
>          }
>          if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)

I think you also want to get rid of the c_info->type check?

> -            libxl_defbool_set(&b_info->u.pv.e820_host, true);
> +            libxl_defbool_set(&b_info->e820_host, true);
>      }
> 
>      switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
> 
> 
> This should maintain the old behaviour for backward compatibility
> when e820_host is not set. I just tested it and it works (with
> e820_host=1 I get the previous error, with e820_host=0, everything
> works fine.

I think it might make sense to relax the PV check. That way the only
way e820_host capability gets activated is if a the guest config
has pci=X stanze. But perhaps that _and_ e820_host=1 is what should
be done.

Or maybe a negative check - if 'pci' stanze is there we automatically
turn on e820_host=1 (right now that is how it works). If the user
has thought 'e820_host=0' and 'pci=xxx' then we would turn the E820
off? That way if something is odd we can turn this off?

> 
> I will have a play with the other two patches tomorrow.
> 
> Gordan