Xen devel - Nov 2013 - [PATCH 0/2] Handle xen_platform_pci=0 case

Hi,

Here is a little series that attempt to fix the issue regarding
xen_platform_pci=0 not been handled.

There are two patches, the first one adds an option to specifies the QEMU
machine that a user wants and we handle the xen_platform_pci=0 case using the
new option.

The new options "qemu_machine_override" will help if one want to try a
q35
based device model. Otherwise, it will be used by libxl to switch to the
"pc"
machine instead of the "xenfv" one when necessary, as the only
difference
between both (since QEMU 1.6) is the presence of the xen-platform pci device.

Regards,

Anthony PERARD (2):
  libxl: adding support to use -machine option of QEMU.
  libxl: Handle xen_platform_pci=0 case with qemu-xen.

 tools/libxl/libxl_dm.c      | 31 +++++++++++++++++++++++++++++--
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  3 +++
 3 files changed, 33 insertions(+), 2 deletions(-)

-- 
Anthony PERARD

It adds a new config option, "qemu_machine_override".

This can be used in the future to switch to a Q35 based device model. It
can also be used on a recent QEMU to avoid adding the xen-platform
device used to setup PV drivers in the guest.

Two possible values for now are "pc" or "xenfv" but there
are
equivalents and xenfv is the default.  A possible future use could be
qemu_machine_override="q35" when this machine will be able to start
successfully on Xen.

When passed to qemu, libxl automatically adds ",accel=xen" to the
machine option.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c      | 7 +++++--
 tools/libxl/libxl_types.idl | 1 +
 tools/libxl/xl_cmdimpl.c    | 3 +++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 292e351..3d4a913 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -608,7 +608,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
     }
     for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
         flexarray_append(dm_args, b_info->extra[i]);
-    flexarray_append(dm_args, "-M");
+    flexarray_append(dm_args, "-machine");
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_PV:
         flexarray_append(dm_args, "xenpv");
@@ -616,7 +616,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
             flexarray_append(dm_args, b_info->extra_pv[i]);
         break;
     case LIBXL_DOMAIN_TYPE_HVM:
-        flexarray_append(dm_args, "xenfv");
+        if (b_info->qemu_machine)
+            flexarray_append(dm_args, GCSPRINTF("%s,accel=xen",
b_info->qemu_machine));
+        else
+            flexarray_append(dm_args, "xenfv");
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] !=
NULL; i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);
         break;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index cba8eff..4141501 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -317,6 +317,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
     # if you set device_model you must set device_model_version too
     ("device_model",     string),
     ("device_model_ssidref", uint32),
+    ("qemu_machine",     string),
 
     # extra parameters pass directly to qemu, NULL terminated
     ("extra",            libxl_string_list),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 341863e..4de6858 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1491,6 +1491,9 @@ skip_vfb:
     }
 
 
+    xlu_cfg_replace_string (config, "qemu_machine_override",
+                            &b_info->qemu_machine, 0);
+
     xlu_cfg_replace_string (config, "device_model_override",
                             &b_info->device_model, 0);
     if (!xlu_cfg_get_string (config, "device_model_version",
&buf, 0)) {
-- 
Anthony PERARD

This should result in QEMU *not* adding the xen-platform device.

Since QEMU 1.6, this can be achieved by using a different qemu machine.
The one used by libxl is "xenfv", but using QEMU >=1.6 with
"-machine
pc,accel=xen" works as well with only one difference compared to
"xenfv", there is no xen-platform device.

One more things, if "qemu_machine_override" is set by the user, then
we
check if it''s necessary to add the xen-platform device.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3d4a913..33caa2b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -390,6 +390,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
     const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
     const libxl_sdl_info *sdl = dm_sdl(guest_config);
     const char *keymap = dm_keymap(guest_config);
+    const char *machine = b_info->qemu_machine;
     flexarray_t *dm_args;
     int i;
     uint64_t ram_size;
@@ -608,6 +609,29 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
     }
     for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
         flexarray_append(dm_args, b_info->extra[i]);
+
+    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        if (libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
+            if (b_info->qemu_machine) {
+                /* Check the qemu machine asked for, it can be
"xenfv" which
+                 * will add xen-platform, or it can be "pc" or
"pc-*" which
+                 * in this case will need to add the device here. Anything
+                 * else is either a mistake or a machine not supported by
+                 * xen. */
+                if (!strncmp("pc", b_info->qemu_machine, 2)) {
+                    flexarray_vappend(dm_args, "-device",
"xen-platform");
+                }
+            }
+        } else {
+            /* Switching here to the machine "pc" which does not add
+             * the xen-platform device instead of the default "xenfv"
machine.
+             */
+            if (!b_info->qemu_machine) {
+                machine = "pc";
+            }
+        }
+    }
+
     flexarray_append(dm_args, "-machine");
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_PV:
@@ -616,8 +640,8 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
             flexarray_append(dm_args, b_info->extra_pv[i]);
         break;
     case LIBXL_DOMAIN_TYPE_HVM:
-        if (b_info->qemu_machine)
-            flexarray_append(dm_args, GCSPRINTF("%s,accel=xen",
b_info->qemu_machine));
+        if (machine)
+            flexarray_append(dm_args, GCSPRINTF("%s,accel=xen",
machine));
         else
             flexarray_append(dm_args, "xenfv");
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] !=
NULL; i++)
-- 
Anthony PERARD

Processing commands for xen@bugs.xenproject.org:
> graft 20 ^
Graft
`<1385133191-23033-1-git-send-email-anthony.perard@citrix.com>''
onto #20
> thanks
Finished processing.

Modified/created Bugs:
 - 20: http://bugs.xenproject.org/xen/bug/20

---
Xen Hypervisor Bug Tracker
See http://wiki.xen.org/wiki/Reporting_Bugs_against_Xen for information on
reporting bugs
Contact xen-bugs-owner@bugs.xenproject.org with any infrastructure issues

Il 22/11/2013 16:13, Anthony PERARD ha scritto:
> Hi,
>
> Here is a little series that attempt to fix the issue regarding
> xen_platform_pci=0 not been handled.
>
> There are two patches, the first one adds an option to specifies the QEMU
> machine that a user wants and we handle the xen_platform_pci=0 case using
the
> new option.
>
> The new options "qemu_machine_override" will help if one want to
try a q35
> based device model. Otherwise, it will be used by libxl to switch to the
"pc"
> machine instead of the "xenfv" one when necessary, as the only
difference
> between both (since QEMU 1.6) is the presence of the xen-platform pci
device.
>
> Regards,
>
> Anthony PERARD (2):
>    libxl: adding support to use -machine option of QEMU.
>    libxl: Handle xen_platform_pci=0 case with qemu-xen.
>
>   tools/libxl/libxl_dm.c      | 31 +++++++++++++++++++++++++++++--
>   tools/libxl/libxl_types.idl |  1 +
>   tools/libxl/xl_cmdimpl.c    |  3 +++
>   3 files changed, 33 insertions(+), 2 deletions(-)
>
Tested with upstream qemu 1.6 from staging:
vi Config.mk
QEMU_UPSTREAM_URL ?= 
git://xenbits.xen.org/staging/qemu-upstream-unstable.git
QEMU_UPSTREAM_REVISION ?= master

With xen_platform_pci=0 Fedora19 hvm domU gave me kernel panic on boot.
xl create -vvv, xl dmesg and serial domUs boot (with calltrace) logs on 
attachments.

With xen_platform_pci=1 boot correctly.






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

On Fri, 2013-11-22 at 15:13 +0000, Anthony PERARD wrote:
> There are two patches, the first one adds an option to specifies the QEMU
> machine that a user wants and we handle the xen_platform_pci=0 case using
the
> new option.
I''m thinking we should make this new option an enum of actually
supported machine types, rather than just a free reign. Makes more sense
from a "what do we support" PoV.
> 
> The new options "qemu_machine_override" will help if one want to
try a q35
> based device model. Otherwise, it will be used by libxl to switch to the
"pc"
> machine instead of the "xenfv" one when necessary, as the only
difference
> between both (since QEMU 1.6) is the presence of the xen-platform pci
device.
> 
> Regards,
> 
> Anthony PERARD (2):
>   libxl: adding support to use -machine option of QEMU.
>   libxl: Handle xen_platform_pci=0 case with qemu-xen.
> 
>  tools/libxl/libxl_dm.c      | 31 +++++++++++++++++++++++++++++--
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    |  3 +++
>  3 files changed, 33 insertions(+), 2 deletions(-)
>

On Fri, Nov 22, 2013 at 04:49:06PM +0100, Fabio Fantoni
wrote:
> Il 22/11/2013 16:13, Anthony PERARD ha scritto:
> >Hi,
> >
> >Here is a little series that attempt to fix the issue regarding
> >xen_platform_pci=0 not been handled.
> >
> >There are two patches, the first one adds an option to specifies the
QEMU
> >machine that a user wants and we handle the xen_platform_pci=0 case
using the
> >new option.
> >
> >The new options "qemu_machine_override" will help if one want
to try a q35
> >based device model. Otherwise, it will be used by libxl to switch to
the "pc"
> >machine instead of the "xenfv" one when necessary, as the
only difference
> >between both (since QEMU 1.6) is the presence of the xen-platform pci
device.
> >
> >Regards,
> >
> >Anthony PERARD (2):
> >   libxl: adding support to use -machine option of QEMU.
> >   libxl: Handle xen_platform_pci=0 case with qemu-xen.
> >
> >  tools/libxl/libxl_dm.c      | 31 +++++++++++++++++++++++++++++--
> >  tools/libxl/libxl_types.idl |  1 +
> >  tools/libxl/xl_cmdimpl.c    |  3 +++
> >  3 files changed, 33 insertions(+), 2 deletions(-)
> >
> 
> Tested with upstream qemu 1.6 from staging:
> vi Config.mk
> QEMU_UPSTREAM_URL ?>
git://xenbits.xen.org/staging/qemu-upstream-unstable.git
> QEMU_UPSTREAM_REVISION ?= master
> 
> With xen_platform_pci=0 Fedora19 hvm domU gave me kernel panic on boot.
> xl create -vvv, xl dmesg and serial domUs boot (with calltrace) logs on
> attachments.
> 
> With xen_platform_pci=1 boot correctly.
> 
> 
> [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-3.11.6-200.fc19.x86_64
root=/d
> ev/mapper/fedora-root ro rd.lvm.lv=fedora/swap rd.md=0 rd.dm=0 rd.luks=0
vconso
> le.font=latarcyrheb-sun16 rd.lvm.lv=fedora/root vconsole.keymap=it2 rhgb
debug
> console=hvc0 console=ttyS0 LANG=it_IT.UTF-8
[...]
> [    1.575205] scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK   
1.6. P
> Q: 0 ANSI: 5
> [    1.575328] sd 0:0:0:0: Attached scsi generic sg0 type 0
> [    1.575381] sd 0:0:0:0: [sda] 20480000 512-byte logical blocks: (10.4
GB/9.7
> 6 GiB)
> [    1.575427] sd 0:0:0:0: [sda] Write Protect is off
> [    1.575429] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> [    1.575445] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,
doe
> sn''t support DPO or FUA
> [    1.661471]  sda: sda1 sda2
> [    1.665373] ------------[ cut here ]------------
> [    1.665704] sd 0:0:0:0: [sda] Attached SCSI disk
> [    1.666342] kernel BUG at drivers/xen/grant-table.c:1189!
> [    1.666342] invalid opcode: 0000 [#1] SMP 
> [    1.666342] Modules linked in:
> [    1.666342] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.11.6-200.fc19.x86_64
>  #1
> [    1.666342] Hardware name: Xen HVM domU, BIOS 4.4-unstable 11/22/2013
> [    1.666342] task: ffff88007a7b0000 ti: ffff88007a7b8000 task.ti:
ffff88007a7
> b8000
> [    1.666342] RIP: 0010:[<ffffffff813a0c0f>] 
[<ffffffff813a0c0f>] get_free_en
> tries+0x2cf/0x2e0
> [    1.666342] RSP: 0000:ffff88007a7b9bd8  EFLAGS: 00010046
> [    1.666342] RAX: 0000000000000286 RBX: 0000000000000001 RCX:
000000000000000
> 0
> [    1.666342] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
ffffffff81fc7d7
> 0
> [    1.666342] RBP: ffff88007a7b9c20 R08: 0000000000000000 R09:
000000000000fff
> c
> [    1.666342] R10: 0000000000000000 R11: 0000000000000000 R12:
000000000000028
> 6
> [    1.666342] R13: 0000000000037032 R14: 0000000000000000 R15:
000000000000000
> 0
> [    1.666342] FS:  0000000000000000(0000) GS:ffff88007b600000(0000)
knlGS:0000
> 000000000000
> [    1.666342] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [    1.666342] CR2: 0000000000000000 CR3: 0000000001c0c000 CR4:
00000000000006f
> 0
> [    1.666342] Stack:
> [    1.666342]  ffff8800370174b0 ffff880037017408 ffff88007a7b9c20
00000001814b
> 68b6
> [    1.666342]  ffff880037024600 0000000000000000 0000000000037032
000000000000
> 0000
> [    1.666342]  0000000000000000 ffff88007a7b9c50 ffffffff813a0c43
ffff88003702
> 4600
> [    1.666342] Call Trace:
> [    1.666342]  [<ffffffff813a0c43>]
gnttab_grant_foreign_access+0x23/0x60
> [    1.666342]  [<ffffffff814c7e58>]
xenkbd_connect_backend+0x58/0x2c0
> [    1.666342]  [<ffffffff814c848a>] xenkbd_probe+0x2fa/0x340
> [    1.666342]  [<ffffffff813a856e>] xenbus_dev_probe+0x8e/0x170
> [    1.666342]  [<ffffffff813a9ba8>]
xenbus_frontend_dev_probe+0x48/0x50
> [    1.666342]  [<ffffffff813f21a7>] driver_probe_device+0x87/0x390
> [    1.666342]  [<ffffffff813f2583>] __driver_attach+0x93/0xa0
> [    1.666342]  [<ffffffff813f24f0>] ? __device_attach+0x40/0x40
> [    1.666342]  [<ffffffff813f00e3>] bus_for_each_dev+0x63/0xa0
> [    1.666342]  [<ffffffff813f1bfe>] driver_attach+0x1e/0x20
> [    1.666342]  [<ffffffff813f1798>] bus_add_driver+0x1e8/0x2a0
> [    1.666342]  [<ffffffff81d59578>] ? lifebook_module_init+0x1b/0x1b
> [    1.666342]  [<ffffffff813f2bc4>] driver_register+0x74/0x150
> [    1.666342]  [<ffffffff81d59578>] ? lifebook_module_init+0x1b/0x1b
> [    1.666342]  [<ffffffff813a7daa>]
xenbus_register_driver_common+0x1a/0x20
> [    1.666342]  [<ffffffff813aa078>]
xenbus_register_frontend+0x28/0x50
> [    1.666342]  [<ffffffff81d595a3>] xenkbd_init+0x2b/0x34
> [    1.666342]  [<ffffffff810020fa>] do_one_initcall+0xfa/0x1b0
> [    1.666342]  [<ffffffff81086785>] ? parse_args+0x225/0x400
> [    1.666342]  [<ffffffff81d0f078>] kernel_init_freeable+0x177/0x1ff
> [    1.666342]  [<ffffffff81d0e898>] ? do_early_param+0x88/0x88
> [    1.666342]  [<ffffffff8163df40>] ? rest_init+0x80/0x80
> [    1.666342]  [<ffffffff8163df4e>] kernel_init+0xe/0x190
> [    1.666342]  [<ffffffff81656dec>] ret_from_fork+0x7c/0xb0
> [    1.666342]  [<ffffffff8163df40>] ? rest_init+0x80/0x80
> [    1.666342] Code: 8b 05 9e 71 c2 00 44 8b 2d 83 71 c2 00 e9 62 fe ff ff
66 2
> e 0f 1f 84 00 00 00 00 00 b8 e4 ff ff ff e9 2a ff ff ff 44 89 c7 eb 84
<0f> 0b
> 0f 0b 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 
> [    1.666342] RIP  [<ffffffff813a0c0f>] get_free_entries+0x2cf/0x2e0
> [    1.666342]  RSP <ffff88007a7b9bd8>
> [    1.666342] ---[ end trace 6eb44b05748c7d42 ]---
> [    2.122137] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0
> 000000b
Looks like I have a similaire issue with the module xen_kbdfront, but
later in the boot, an the guest is running fine.  I think that could be
an hidden bug which did not show up before, I''ll try to understand the
issue.

Thank you for testing.

-- 
Anthony PERARD

On 22/11/13 15:13, Anthony PERARD wrote:
> Hi,
>
> Here is a little series that attempt to fix the issue regarding
> xen_platform_pci=0 not been handled.
>
> There are two patches, the first one adds an option to specifies the QEMU
> machine that a user wants and we handle the xen_platform_pci=0 case using
the
> new option.
>
> The new options "qemu_machine_override" will help if one want to
try a q35
> based device model. Otherwise, it will be used by libxl to switch to the
"pc"
> machine instead of the "xenfv" one when necessary, as the only
difference
> between both (since QEMU 1.6) is the presence of the xen-platform pci
device.
>
> Regards,
>
> Anthony PERARD (2):
>    libxl: adding support to use -machine option of QEMU.
>    libxl: Handle xen_platform_pci=0 case with qemu-xen.
As I understand it, this is a fix to functionality which should be 
supported, so doesn''t need a freeze exception at this point.

  -George

On Fri, Nov 22, 2013 at 03:56:13PM +0000, Ian Campbell
wrote:
> On Fri, 2013-11-22 at 15:13 +0000, Anthony PERARD wrote:
> > There are two patches, the first one adds an option to specifies the
QEMU
> > machine that a user wants and we handle the xen_platform_pci=0 case
using the
> > new option.
> 
> I''m thinking we should make this new option an enum of actually
> supported machine types, rather than just a free reign. Makes more sense
> from a "what do we support" PoV.
And start with the following list ?
pc
pc-i440fx-1.7
pc-i440fx-1.6
pc-i440fx-1.5
pc-i440fx-1.4
pc-1.3
pc-1.2
pc-1.1
pc-1.0
pc-0.15
pc-0.14
pc-0.13
pc-0.12
pc-0.11
pc-0.10
xenfv

I wonder how many of this will work. I suspect most of them. The only
down side is that it will be necessary to patch libxl if one want to
try, let say q35.

If we only keep xenfv and pc in the list, it might be better to just not
expose the option to the config file, for now.

But yes, having a list for the supported feature point of view will be
better.

-- 
Anthony PERARD

On Fri, 2013-11-22 at 17:18 +0000, Anthony PERARD wrote:
> On Fri, Nov 22, 2013 at 03:56:13PM +0000, Ian Campbell wrote:
> > On Fri, 2013-11-22 at 15:13 +0000, Anthony PERARD wrote:
> > > There are two patches, the first one adds an option to specifies
the QEMU
> > > machine that a user wants and we handle the xen_platform_pci=0
case using the
> > > new option.
> > 
> > I''m thinking we should make this new option an enum of
actually
> > supported machine types, rather than just a free reign. Makes more
sense
> > from a "what do we support" PoV.
> 
> And start with the following list ?
Uh, that''s quite long.
> pc
> pc-i440fx-1.7
> pc-i440fx-1.6
> pc-i440fx-1.5
> pc-i440fx-1.4
> pc-1.3
> pc-1.2
> pc-1.1
> pc-1.0
> pc-0.15
> pc-0.14
> pc-0.13
> pc-0.12
> pc-0.11
> pc-0.10
> xenfv
> 
> I wonder how many of this will work. I suspect most of them.
Work != Warantied by Xen.org (whatever that means)
>  The only
> down side is that it will be necessary to patch libxl if one want to
> try, let say q35.
> 
> If we only keep xenfv and pc in the list, it might be better to just not
> expose the option to the config file, for now.
> 
> But yes, having a list for the supported feature point of view will be
> better.
I can just about see having a non "_override" enum *and* and _override
string. But ick.

Ian.

On 22/11/13 17:18, Anthony PERARD wrote:
> On Fri, Nov 22, 2013 at 03:56:13PM +0000, Ian Campbell wrote:
>> On Fri, 2013-11-22 at 15:13 +0000, Anthony PERARD wrote:
>>> There are two patches, the first one adds an option to specifies
the QEMU
>>> machine that a user wants and we handle the xen_platform_pci=0 case
using the
>>> new option.
>> I''m thinking we should make this new option an enum of
actually
>> supported machine types, rather than just a free reign. Makes more
sense
>> from a "what do we support" PoV.
> And start with the following list ?
> pc
> pc-i440fx-1.7
> pc-i440fx-1.6
> pc-i440fx-1.5
> pc-i440fx-1.4
> pc-1.3
> pc-1.2
> pc-1.1
> pc-1.0
> pc-0.15
> pc-0.14
> pc-0.13
> pc-0.12
> pc-0.11
> pc-0.10
> xenfv
>
> I wonder how many of this will work. I suspect most of them. The only
> down side is that it will be necessary to patch libxl if one want to
> try, let say q35.
In a way I think that''s the point -- by allowing the user to specify an
arbitrary sting, we are implicitly supporting a gigantic range of things 
which may work in one release or in one particular configuration and 
then break in a different one.  By making an enum, we bake in the exact 
ones which we consider to be valid.

Also, we''ve been trying to get away from exposing a bunch of qemu-isms 
to the user via libxl and the xl config file.  What might the *user* be 
trying to do in specifying this option?  We should make sure the 
interface specifies things the user actually wants, and then use qemu''s
options to implement that, rather than asking the user to specify qemu 
stuff directly.

  -George

On Fri, 2013-11-22 at 17:23 +0000, Ian Campbell wrote:
> I can just about see having a non "_override" enum *and* and
_override
> string. But ick.
Maybe the string override can be via device_model_args?

Ian.

On Fri, Nov 22, 2013 at 05:31:19PM +0000, Ian Campbell
wrote:
> On Fri, 2013-11-22 at 17:23 +0000, Ian Campbell wrote:
> 
> > I can just about see having a non "_override" enum *and* and
_override
> > string. But ick.
> 
> Maybe the string override can be via device_model_args?
Looks like it is possible to have an extra -machine in
device_model_args_hvm, and qemu will use the last one. So it is probably
best to not expose the qemu_machine at all.

-- 
Anthony PERARD

Il 22/11/2013 17:54, Anthony PERARD ha scritto:
> On Fri, Nov 22, 2013 at 04:49:06PM +0100, Fabio Fantoni wrote:
>> Il 22/11/2013 16:13, Anthony PERARD ha scritto:
>>> Hi,
>>>
>>> Here is a little series that attempt to fix the issue regarding
>>> xen_platform_pci=0 not been handled.
>>>
>>> There are two patches, the first one adds an option to specifies
the QEMU
>>> machine that a user wants and we handle the xen_platform_pci=0 case
using the
>>> new option.
>>>
>>> The new options "qemu_machine_override" will help if one
want to try a q35
>>> based device model. Otherwise, it will be used by libxl to switch
to the "pc"
>>> machine instead of the "xenfv" one when necessary, as the
only difference
>>> between both (since QEMU 1.6) is the presence of the xen-platform
pci device.
About q35 in theory should be missing only the implementation in 
hvmloader, is there a draft somewhere to try or to should be made?
>>>
>>> Regards,
>>>
>>> Anthony PERARD (2):
>>>    libxl: adding support to use -machine option of QEMU.
>>>    libxl: Handle xen_platform_pci=0 case with qemu-xen.
>>>
>>>   tools/libxl/libxl_dm.c      | 31 +++++++++++++++++++++++++++++--
>>>   tools/libxl/libxl_types.idl |  1 +
>>>   tools/libxl/xl_cmdimpl.c    |  3 +++
>>>   3 files changed, 33 insertions(+), 2 deletions(-)
>>>
>> Tested with upstream qemu 1.6 from staging:
>> vi Config.mk
>> QEMU_UPSTREAM_URL ?>>
git://xenbits.xen.org/staging/qemu-upstream-unstable.git
>> QEMU_UPSTREAM_REVISION ?= master
>>
>> With xen_platform_pci=0 Fedora19 hvm domU gave me kernel panic on boot.
>> xl create -vvv, xl dmesg and serial domUs boot (with calltrace) logs on
>> attachments.
>>
>> With xen_platform_pci=1 boot correctly.
>>
>>
>
>> [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-3.11.6-200.fc19.x86_64
root=/d
>> ev/mapper/fedora-root ro rd.lvm.lv=fedora/swap rd.md=0 rd.dm=0
rd.luks=0 vconso
>> le.font=latarcyrheb-sun16 rd.lvm.lv=fedora/root vconsole.keymap=it2
rhgb debug
>> console=hvc0 console=ttyS0 LANG=it_IT.UTF-8
> [...]
>> [    1.575205] scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK  
1.6. P
>> Q: 0 ANSI: 5
>> [    1.575328] sd 0:0:0:0: Attached scsi generic sg0 type 0
>> [    1.575381] sd 0:0:0:0: [sda] 20480000 512-byte logical blocks:
(10.4 GB/9.7
>> 6 GiB)
>> [    1.575427] sd 0:0:0:0: [sda] Write Protect is off
>> [    1.575429] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>> [    1.575445] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
enabled, doe
>> sn''t support DPO or FUA
>> [    1.661471]  sda: sda1 sda2
>> [    1.665373] ------------[ cut here ]------------
>> [    1.665704] sd 0:0:0:0: [sda] Attached SCSI disk
>> [    1.666342] kernel BUG at drivers/xen/grant-table.c:1189!
>> [    1.666342] invalid opcode: 0000 [#1] SMP
>> [    1.666342] Modules linked in:
>> [    1.666342] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.11.6-200.fc19.x86_64
>>   #1
>> [    1.666342] Hardware name: Xen HVM domU, BIOS 4.4-unstable
11/22/2013
>> [    1.666342] task: ffff88007a7b0000 ti: ffff88007a7b8000 task.ti:
ffff88007a7
>> b8000
>> [    1.666342] RIP: 0010:[<ffffffff813a0c0f>] 
[<ffffffff813a0c0f>] get_free_en
>> tries+0x2cf/0x2e0
>> [    1.666342] RSP: 0000:ffff88007a7b9bd8  EFLAGS: 00010046
>> [    1.666342] RAX: 0000000000000286 RBX: 0000000000000001 RCX:
000000000000000
>> 0
>> [    1.666342] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
ffffffff81fc7d7
>> 0
>> [    1.666342] RBP: ffff88007a7b9c20 R08: 0000000000000000 R09:
000000000000fff
>> c
>> [    1.666342] R10: 0000000000000000 R11: 0000000000000000 R12:
000000000000028
>> 6
>> [    1.666342] R13: 0000000000037032 R14: 0000000000000000 R15:
000000000000000
>> 0
>> [    1.666342] FS:  0000000000000000(0000) GS:ffff88007b600000(0000)
knlGS:0000
>> 000000000000
>> [    1.666342] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>> [    1.666342] CR2: 0000000000000000 CR3: 0000000001c0c000 CR4:
00000000000006f
>> 0
>> [    1.666342] Stack:
>> [    1.666342]  ffff8800370174b0 ffff880037017408 ffff88007a7b9c20
00000001814b
>> 68b6
>> [    1.666342]  ffff880037024600 0000000000000000 0000000000037032
000000000000
>> 0000
>> [    1.666342]  0000000000000000 ffff88007a7b9c50 ffffffff813a0c43
ffff88003702
>> 4600
>> [    1.666342] Call Trace:
>> [    1.666342]  [<ffffffff813a0c43>]
gnttab_grant_foreign_access+0x23/0x60
>> [    1.666342]  [<ffffffff814c7e58>]
xenkbd_connect_backend+0x58/0x2c0
>> [    1.666342]  [<ffffffff814c848a>] xenkbd_probe+0x2fa/0x340
>> [    1.666342]  [<ffffffff813a856e>] xenbus_dev_probe+0x8e/0x170
>> [    1.666342]  [<ffffffff813a9ba8>]
xenbus_frontend_dev_probe+0x48/0x50
>> [    1.666342]  [<ffffffff813f21a7>]
driver_probe_device+0x87/0x390
>> [    1.666342]  [<ffffffff813f2583>] __driver_attach+0x93/0xa0
>> [    1.666342]  [<ffffffff813f24f0>] ? __device_attach+0x40/0x40
>> [    1.666342]  [<ffffffff813f00e3>] bus_for_each_dev+0x63/0xa0
>> [    1.666342]  [<ffffffff813f1bfe>] driver_attach+0x1e/0x20
>> [    1.666342]  [<ffffffff813f1798>] bus_add_driver+0x1e8/0x2a0
>> [    1.666342]  [<ffffffff81d59578>] ?
lifebook_module_init+0x1b/0x1b
>> [    1.666342]  [<ffffffff813f2bc4>] driver_register+0x74/0x150
>> [    1.666342]  [<ffffffff81d59578>] ?
lifebook_module_init+0x1b/0x1b
>> [    1.666342]  [<ffffffff813a7daa>]
xenbus_register_driver_common+0x1a/0x20
>> [    1.666342]  [<ffffffff813aa078>]
xenbus_register_frontend+0x28/0x50
>> [    1.666342]  [<ffffffff81d595a3>] xenkbd_init+0x2b/0x34
>> [    1.666342]  [<ffffffff810020fa>] do_one_initcall+0xfa/0x1b0
>> [    1.666342]  [<ffffffff81086785>] ? parse_args+0x225/0x400
>> [    1.666342]  [<ffffffff81d0f078>]
kernel_init_freeable+0x177/0x1ff
>> [    1.666342]  [<ffffffff81d0e898>] ? do_early_param+0x88/0x88
>> [    1.666342]  [<ffffffff8163df40>] ? rest_init+0x80/0x80
>> [    1.666342]  [<ffffffff8163df4e>] kernel_init+0xe/0x190
>> [    1.666342]  [<ffffffff81656dec>] ret_from_fork+0x7c/0xb0
>> [    1.666342]  [<ffffffff8163df40>] ? rest_init+0x80/0x80
>> [    1.666342] Code: 8b 05 9e 71 c2 00 44 8b 2d 83 71 c2 00 e9 62 fe ff
ff 66 2
>> e 0f 1f 84 00 00 00 00 00 b8 e4 ff ff ff e9 2a ff ff ff 44 89 c7 eb 84
<0f> 0b
>> 0f 0b 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66
>> [    1.666342] RIP  [<ffffffff813a0c0f>]
get_free_entries+0x2cf/0x2e0
>> [    1.666342]  RSP <ffff88007a7b9bd8>
>> [    1.666342] ---[ end trace 6eb44b05748c7d42 ]---
>> [    2.122137] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0
>> 000000b
> Looks like I have a similaire issue with the module xen_kbdfront, but
> later in the boot, an the guest is running fine.  I think that could be
> an hidden bug which did not show up before, I''ll try to understand
the
> issue.
>
> Thank you for testing.
>
If you need more tests and/or informations tell me and I''ll do and post
them.

On Mon, Nov 25, 2013 at 10:04:59AM +0100, Fabio Fantoni
wrote:
> Il 22/11/2013 17:54, Anthony PERARD ha scritto:
> >On Fri, Nov 22, 2013 at 04:49:06PM +0100, Fabio Fantoni wrote:
> >>Il 22/11/2013 16:13, Anthony PERARD ha scritto:
> >>>Hi,
> >>>
> >>>Here is a little series that attempt to fix the issue regarding
> >>>xen_platform_pci=0 not been handled.
> >>>
> >>>There are two patches, the first one adds an option to
specifies the QEMU
> >>>machine that a user wants and we handle the xen_platform_pci=0
case using the
> >>>new option.
> >>>
> >>>The new options "qemu_machine_override" will help if
one want to try a q35
> >>>based device model. Otherwise, it will be used by libxl to
switch to the "pc"
> >>>machine instead of the "xenfv" one when necessary, as
the only difference
> >>>between both (since QEMU 1.6) is the presence of the
xen-platform pci device.
> 
> About q35 in theory should be missing only the implementation in hvmloader,
> is there a draft somewhere to try or to should be made?
No, sorry, no draft. In hvmloader, there is an easy change, comment the
assert that prevent hvmloader from starting, but that only the
beginning.

-- 
Anthony PERARD

> > [    1.665373] ------------[ cut here ]------------
> > [    1.665704] sd 0:0:0:0: [sda] Attached SCSI disk
> > [    1.666342] kernel BUG at drivers/xen/grant-table.c:1189!
> > [    1.666342] invalid opcode: 0000 [#1] SMP 
> > [    1.666342] Modules linked in:
> > [    1.666342] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.11.6-200.fc19.x86_64
> >  #1
> > [    1.666342] Hardware name: Xen HVM domU, BIOS 4.4-unstable
11/22/2013
> > [    1.666342] task: ffff88007a7b0000 ti: ffff88007a7b8000 task.ti:
ffff88007a7
> > b8000
> > [    1.666342] RIP: 0010:[<ffffffff813a0c0f>] 
[<ffffffff813a0c0f>] get_free_en
> > tries+0x2cf/0x2e0
> > [    1.666342] RSP: 0000:ffff88007a7b9bd8  EFLAGS: 00010046
> > [    1.666342] RAX: 0000000000000286 RBX: 0000000000000001 RCX:
000000000000000
> > 0
> > [    1.666342] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
ffffffff81fc7d7
> > 0
> > [    1.666342] RBP: ffff88007a7b9c20 R08: 0000000000000000 R09:
000000000000fff
> > c
> > [    1.666342] R10: 0000000000000000 R11: 0000000000000000 R12:
000000000000028
> > 6
> > [    1.666342] R13: 0000000000037032 R14: 0000000000000000 R15:
000000000000000
> > 0
> > [    1.666342] FS:  0000000000000000(0000) GS:ffff88007b600000(0000)
knlGS:0000
> > 000000000000
> > [    1.666342] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > [    1.666342] CR2: 0000000000000000 CR3: 0000000001c0c000 CR4:
00000000000006f
> > 0
> > [    1.666342] Stack:
> > [    1.666342]  ffff8800370174b0 ffff880037017408 ffff88007a7b9c20
00000001814b
> > 68b6
> > [    1.666342]  ffff880037024600 0000000000000000 0000000000037032
000000000000
> > 0000
> > [    1.666342]  0000000000000000 ffff88007a7b9c50 ffffffff813a0c43
ffff88003702
> > 4600
> > [    1.666342] Call Trace:
> > [    1.666342]  [<ffffffff813a0c43>]
gnttab_grant_foreign_access+0x23/0x60
> > [    1.666342]  [<ffffffff814c7e58>]
xenkbd_connect_backend+0x58/0x2c0
> > [    1.666342]  [<ffffffff814c848a>] xenkbd_probe+0x2fa/0x340
> > [    1.666342]  [<ffffffff813a856e>] xenbus_dev_probe+0x8e/0x170
> > [    1.666342]  [<ffffffff813a9ba8>]
xenbus_frontend_dev_probe+0x48/0x50
> > [    1.666342]  [<ffffffff813f21a7>]
driver_probe_device+0x87/0x390
> > [    1.666342]  [<ffffffff813f2583>] __driver_attach+0x93/0xa0
> > [    1.666342]  [<ffffffff813f24f0>] ? __device_attach+0x40/0x40
> > [    1.666342]  [<ffffffff813f00e3>] bus_for_each_dev+0x63/0xa0
> > [    1.666342]  [<ffffffff813f1bfe>] driver_attach+0x1e/0x20
> > [    1.666342]  [<ffffffff813f1798>] bus_add_driver+0x1e8/0x2a0
> > [    1.666342]  [<ffffffff81d59578>] ?
lifebook_module_init+0x1b/0x1b
> > [    1.666342]  [<ffffffff813f2bc4>] driver_register+0x74/0x150
> > [    1.666342]  [<ffffffff81d59578>] ?
lifebook_module_init+0x1b/0x1b
> > [    1.666342]  [<ffffffff813a7daa>]
xenbus_register_driver_common+0x1a/0x20
> > [    1.666342]  [<ffffffff813aa078>]
xenbus_register_frontend+0x28/0x50
> > [    1.666342]  [<ffffffff81d595a3>] xenkbd_init+0x2b/0x34
> > [    1.666342]  [<ffffffff810020fa>] do_one_initcall+0xfa/0x1b0
> > [    1.666342]  [<ffffffff81086785>] ? parse_args+0x225/0x400
> > [    1.666342]  [<ffffffff81d0f078>]
kernel_init_freeable+0x177/0x1ff
> > [    1.666342]  [<ffffffff81d0e898>] ? do_early_param+0x88/0x88
> > [    1.666342]  [<ffffffff8163df40>] ? rest_init+0x80/0x80
> > [    1.666342]  [<ffffffff8163df4e>] kernel_init+0xe/0x190
> > [    1.666342]  [<ffffffff81656dec>] ret_from_fork+0x7c/0xb0
> > [    1.666342]  [<ffffffff8163df40>] ? rest_init+0x80/0x80
> > [    1.666342] Code: 8b 05 9e 71 c2 00 44 8b 2d 83 71 c2 00 e9 62 fe
ff ff 66 2
> > e 0f 1f 84 00 00 00 00 00 b8 e4 ff ff ff e9 2a ff ff ff 44 89 c7 eb 84
<0f> 0b
> > 0f 0b 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 
> > [    1.666342] RIP  [<ffffffff813a0c0f>]
get_free_entries+0x2cf/0x2e0
> > [    1.666342]  RSP <ffff88007a7b9bd8>
> > [    1.666342] ---[ end trace 6eb44b05748c7d42 ]---
> > [    2.122137] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0
> > 000000b
> 
> Looks like I have a similaire issue with the module xen_kbdfront, but
> later in the boot, an the guest is running fine.  I think that could be
> an hidden bug which did not show up before, I''ll try to understand
the
> issue.
This looks familiar. I thought we had discussed it and it came out of this:

t d0b4d64aadb9f4a90669848de9ef3819050a98cd
Author: Matt Wilson <msw@amazon.com>
Date:   Tue Jan 15 13:21:27 2013 +0000

    xen/grant-table: correctly initialize grant table version 1
    
    Commit 85ff6acb075a484780b3d763fdf41596d8fc0970 (xen/granttable: Grant
    tables V2 implementation) changed the GREFS_PER_GRANT_FRAME macro from
    a constant to a conditional expression. The expression depends on
    grant_table_version being appropriately set. Unfortunately, at init
    time grant_table_version will be 0. The GREFS_PER_GRANT_FRAME
    conditional expression checks for "grant_table_version == 1", and
    therefore returns the number of grant references per frame for v2.
    
    This causes gnttab_init() to allocate fewer pages for gnttab_list, as
    a frame can old half the number of v2 entries than v1 entries. After
    gnttab_resume() is called, grant_table_version is appropriately
    set. nr_init_grefs will then be miscalculated and gnttab_free_count
    will hold a value larger than the actual number of free gref entries.
    
    If a guest is heavily utilizing improperly initialized v1 grant
    tables, memory corruption can occur. One common manifestation is
    corruption of the vmalloc list, resulting in a poisoned pointer
    derefrence when accessing /proc/meminfo or /proc/vmallocinfo:


And the reason is that since the xen-platform-pci is not called
we never called ''gnttab_setup'' (or rather
''gnttab_init()'').

I am not really sure how to fix this right - we don''t want
to load any of the PV drivers, but at the same time the
''xen_domain()''
check is turned ''on'' by:

xen_hvm_guest_init().

which is OK.

But if we add in the PV drivers a check for:

	if (xen_domain()) {
		if (xen_hvm_domain() && !xen-platform-pci-init-value))
			return -ENODEV;
	}

that means we won''t be able to load the drivers in random order. Such
as xen-blkfront loaded before xen-platform-pci. But that is OK, those
modules have a dependency on that driver (I hope!).

Something like this? (Untested, not even compiled)

diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
index 0a78524..3e480f1 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -69,6 +69,22 @@ static int check_platform_magic(void)
 	return 0;
 }
 
+bool xen_err_out(void)
+{
+	if (!xen_domain())
+		return true;
+
+       if (xen_hvm_domain()) {
+		if (xen_platform_pci_unplug & (XEN_UNPLUG_UNNECESSARY |
XEN_UNPLUG_NEVER))
+			return true;
+		if (xen_platform_pci_unplug == 0)
+			return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(xen_err_out);
+
+}
 void xen_unplug_emulated_devices(void)
 {
 	int r;
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 432db1b..bcbaf0b 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2074,7 +2074,7 @@ static int __init xlblk_init(void)
 	if (!xen_domain())
 		return -ENODEV;
 
-	if (xen_hvm_domain() && !xen_platform_pci_unplug)
+	if (xen_err_out())
 		return -ENODEV;
 
 	if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) {
diff --git a/drivers/input/misc/xen-kbdfront.c
b/drivers/input/misc/xen-kbdfront.c
index e21c181..e3640d6 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -380,6 +380,9 @@ static int __init xenkbd_init(void)
 	if (xen_initial_domain())
 		return -ENODEV;
 
+	if (xen_err_out())
+		return -ENODEV;
+
 	return xenbus_register_frontend(&xenkbd_driver);
 }
 
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e59acb1..be2744b 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2115,7 +2115,7 @@ static int __init netif_init(void)
 	if (!xen_domain())
 		return -ENODEV;
 
-	if (xen_hvm_domain() && !xen_platform_pci_unplug)
+	if (xen_err_out())
 		return -ENODEV;
 
 	pr_info("Initialising Xen virtual ethernet driver\n");
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 129bf84..b1c0f2a 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init);
 #ifndef MODULE
 static int __init boot_wait_for_devices(void)
 {
-	if (xen_hvm_domain() && !xen_platform_pci_unplug)
+	if (xen_err_out())
 		return -ENODEV;
 
 	ready_to_wait_for_devices = 1;
diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index 438c256..2120f90 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -47,5 +47,6 @@ static inline int xen_must_unplug_disks(void) {
 }
 
 extern int xen_platform_pci_unplug;
+extern bool xen_err_out(void);
 
 #endif /* _XEN_PLATFORM_PCI_H */

> Something like this? (Untested, not even compiled)
> 
Please try this one:

From f7d3581aa19a35ea3ff10b965b2b08843e923635 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 26 Nov 2013 15:05:40 -0500
Subject: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.

*TODO*

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c               |  2 +-
 drivers/input/misc/xen-kbdfront.c          |  4 ++++
 drivers/net/xen-netfront.c                 |  2 +-
 drivers/xen/xenbus/xenbus_probe_frontend.c |  2 +-
 include/xen/platform_pci.h                 | 13 +++++++++++++
 5 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 432db1b..bcbaf0b 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2074,7 +2074,7 @@ static int __init xlblk_init(void)
 	if (!xen_domain())
 		return -ENODEV;
 
-	if (xen_hvm_domain() && !xen_platform_pci_unplug)
+	if (xen_err_out())
 		return -ENODEV;
 
 	if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) {
diff --git a/drivers/input/misc/xen-kbdfront.c
b/drivers/input/misc/xen-kbdfront.c
index e21c181..9d250cf 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -29,6 +29,7 @@
 #include <xen/interface/io/fbif.h>
 #include <xen/interface/io/kbdif.h>
 #include <xen/xenbus.h>
+#include <xen/platform_pci.h>
 
 struct xenkbd_info {
 	struct input_dev *kbd;
@@ -380,6 +381,9 @@ static int __init xenkbd_init(void)
 	if (xen_initial_domain())
 		return -ENODEV;
 
+	if (xen_err_out())
+		return -ENODEV;
+
 	return xenbus_register_frontend(&xenkbd_driver);
 }
 
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e59acb1..be2744b 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2115,7 +2115,7 @@ static int __init netif_init(void)
 	if (!xen_domain())
 		return -ENODEV;
 
-	if (xen_hvm_domain() && !xen_platform_pci_unplug)
+	if (xen_err_out())
 		return -ENODEV;
 
 	pr_info("Initialising Xen virtual ethernet driver\n");
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 129bf84..b1c0f2a 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init);
 #ifndef MODULE
 static int __init boot_wait_for_devices(void)
 {
-	if (xen_hvm_domain() && !xen_platform_pci_unplug)
+	if (xen_err_out())
 		return -ENODEV;
 
 	ready_to_wait_for_devices = 1;
diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index 438c256..a5bbd0b 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -47,5 +47,18 @@ static inline int xen_must_unplug_disks(void) {
 }
 
 extern int xen_platform_pci_unplug;
+static  bool xen_err_out(void)
+{
 
+	if (!xen_domain())
+		return true;
+
+	if (xen_hvm_domain()) {
+		if (xen_platform_pci_unplug & (XEN_UNPLUG_UNNECESSARY |
XEN_UNPLUG_NEVER))
+			return true;
+		if (xen_platform_pci_unplug == 0)
+			return true;
+	}
+	return false;
+}
 #endif /* _XEN_PLATFORM_PCI_H */
-- 
1.8.3.1

On Tue, Nov 26, 2013 at 03:09:56PM -0500, Konrad Rzeszutek Wilk
wrote:
> > Something like this? (Untested, not even compiled)
> > 
> 
> Please try this one:
> 
> From f7d3581aa19a35ea3ff10b965b2b08843e923635 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Tue, 26 Nov 2013 15:05:40 -0500
> Subject: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t
blow up.
> 
> *TODO*
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Looks like Linux is more happy with this patch.

I have apply it on top of Linux 3.12.1 and no more "kernel BUG at
drivers/xen/grant-table.c:1189!" in linux debug. Also, with the patch, my
guest will shutdown (without, something prevent from shuting down
complitly).

So, this patch is working for me. Thanks.
> ---
>  drivers/block/xen-blkfront.c               |  2 +-
>  drivers/input/misc/xen-kbdfront.c          |  4 ++++
>  drivers/net/xen-netfront.c                 |  2 +-
>  drivers/xen/xenbus/xenbus_probe_frontend.c |  2 +-
>  include/xen/platform_pci.h                 | 13 +++++++++++++
>  5 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 432db1b..bcbaf0b 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -2074,7 +2074,7 @@ static int __init xlblk_init(void)
>  	if (!xen_domain())
>  		return -ENODEV;
>  
> -	if (xen_hvm_domain() && !xen_platform_pci_unplug)
> +	if (xen_err_out())
>  		return -ENODEV;
>  
>  	if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) {
> diff --git a/drivers/input/misc/xen-kbdfront.c
b/drivers/input/misc/xen-kbdfront.c
> index e21c181..9d250cf 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -29,6 +29,7 @@
>  #include <xen/interface/io/fbif.h>
>  #include <xen/interface/io/kbdif.h>
>  #include <xen/xenbus.h>
> +#include <xen/platform_pci.h>
>  
>  struct xenkbd_info {
>  	struct input_dev *kbd;
> @@ -380,6 +381,9 @@ static int __init xenkbd_init(void)
>  	if (xen_initial_domain())
>  		return -ENODEV;
>  
> +	if (xen_err_out())
> +		return -ENODEV;
> +
>  	return xenbus_register_frontend(&xenkbd_driver);
>  }
>  
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index e59acb1..be2744b 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -2115,7 +2115,7 @@ static int __init netif_init(void)
>  	if (!xen_domain())
>  		return -ENODEV;
>  
> -	if (xen_hvm_domain() && !xen_platform_pci_unplug)
> +	if (xen_err_out())
>  		return -ENODEV;
>  
>  	pr_info("Initialising Xen virtual ethernet driver\n");
> diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
> index 129bf84..b1c0f2a 100644
> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> @@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init);
>  #ifndef MODULE
>  static int __init boot_wait_for_devices(void)
>  {
> -	if (xen_hvm_domain() && !xen_platform_pci_unplug)
> +	if (xen_err_out())
>  		return -ENODEV;
>  
>  	ready_to_wait_for_devices = 1;
> diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
> index 438c256..a5bbd0b 100644
> --- a/include/xen/platform_pci.h
> +++ b/include/xen/platform_pci.h
> @@ -47,5 +47,18 @@ static inline int xen_must_unplug_disks(void) {
>  }
>  
>  extern int xen_platform_pci_unplug;
> +static  bool xen_err_out(void)
> +{
>  
> +	if (!xen_domain())
> +		return true;
> +
> +	if (xen_hvm_domain()) {
> +		if (xen_platform_pci_unplug & (XEN_UNPLUG_UNNECESSARY |
XEN_UNPLUG_NEVER))
> +			return true;
> +		if (xen_platform_pci_unplug == 0)
> +			return true;
> +	}
> +	return false;
> +}
>  #endif /* _XEN_PLATFORM_PCI_H */
> -- 
> 1.8.3.1
> 
-- 
Anthony PERARD

On Fri, 22 Nov 2013, Anthony PERARD wrote:
> It adds a new config option, "qemu_machine_override".
> 
> This can be used in the future to switch to a Q35 based device model. It
> can also be used on a recent QEMU to avoid adding the xen-platform
> device used to setup PV drivers in the guest.
> 
> Two possible values for now are "pc" or "xenfv" but
there are
> equivalents and xenfv is the default.  A possible future use could be
> qemu_machine_override="q35" when this machine will be able to
start
> successfully on Xen.
> 
> When passed to qemu, libxl automatically adds ",accel=xen" to the
> machine option.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Given that you are introducing a new xl option, shouldn''t you add
documention for it as part of this patch?

>  tools/libxl/libxl_dm.c      | 7 +++++--
>  tools/libxl/libxl_types.idl | 1 +
>  tools/libxl/xl_cmdimpl.c    | 3 +++
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 292e351..3d4a913 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -608,7 +608,7 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>      }
>      for (i = 0; b_info->extra && b_info->extra[i] != NULL;
i++)
>          flexarray_append(dm_args, b_info->extra[i]);
> -    flexarray_append(dm_args, "-M");
> +    flexarray_append(dm_args, "-machine");
>      switch (b_info->type) {
>      case LIBXL_DOMAIN_TYPE_PV:
>          flexarray_append(dm_args, "xenpv");
> @@ -616,7 +616,10 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, b_info->extra_pv[i]);
>          break;
>      case LIBXL_DOMAIN_TYPE_HVM:
> -        flexarray_append(dm_args, "xenfv");
> +        if (b_info->qemu_machine)
> +            flexarray_append(dm_args, GCSPRINTF("%s,accel=xen",
b_info->qemu_machine));
> +        else
> +            flexarray_append(dm_args, "xenfv");
>          for (i = 0; b_info->extra_hvm &&
b_info->extra_hvm[i] != NULL; i++)
>              flexarray_append(dm_args, b_info->extra_hvm[i]);
>          break;
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index cba8eff..4141501 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -317,6 +317,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>      # if you set device_model you must set device_model_version too
>      ("device_model",     string),
>      ("device_model_ssidref", uint32),
> +    ("qemu_machine",     string),
>  
>      # extra parameters pass directly to qemu, NULL terminated
>      ("extra",            libxl_string_list),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 341863e..4de6858 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1491,6 +1491,9 @@ skip_vfb:
>      }
>  
>  
> +    xlu_cfg_replace_string (config, "qemu_machine_override",
> +                            &b_info->qemu_machine, 0);
> +
>      xlu_cfg_replace_string (config, "device_model_override",
>                              &b_info->device_model, 0);
>      if (!xlu_cfg_get_string (config, "device_model_version",
&buf, 0)) {
> -- 
> Anthony PERARD
>

On Fri, 22 Nov 2013, Anthony PERARD wrote:
> This should result in QEMU *not* adding the xen-platform device.
> 
> Since QEMU 1.6, this can be achieved by using a different qemu machine.
> The one used by libxl is "xenfv", but using QEMU >=1.6 with
"-machine
> pc,accel=xen" works as well with only one difference compared to
> "xenfv", there is no xen-platform device.
> 
> One more things, if "qemu_machine_override" is set by the user,
then we
> check if it''s necessary to add the xen-platform device.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_dm.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3d4a913..33caa2b 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -390,6 +390,7 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>      const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
>      const libxl_sdl_info *sdl = dm_sdl(guest_config);
>      const char *keymap = dm_keymap(guest_config);
> +    const char *machine = b_info->qemu_machine;
>      flexarray_t *dm_args;
>      int i;
>      uint64_t ram_size;
> @@ -608,6 +609,29 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>      }
>      for (i = 0; b_info->extra && b_info->extra[i] != NULL;
i++)
>          flexarray_append(dm_args, b_info->extra[i]);
> +
> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> +        if (libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
> +            if (b_info->qemu_machine) {
> +                /* Check the qemu machine asked for, it can be
"xenfv" which
> +                 * will add xen-platform, or it can be "pc" or
"pc-*" which
> +                 * in this case will need to add the device here. Anything
> +                 * else is either a mistake or a machine not supported by
> +                 * xen. */
in that case it would be appropriate to print a warning or an error

> +                if (!strncmp("pc", b_info->qemu_machine, 2))
{
> +                    flexarray_vappend(dm_args, "-device",
"xen-platform");
> +                }
> +            }
> +        } else {
> +            /* Switching here to the machine "pc" which does not
add
> +             * the xen-platform device instead of the default
"xenfv" machine.
> +             */
> +            if (!b_info->qemu_machine) {
> +                machine = "pc";
> +            }
> +        }
> +    }
> +
>      flexarray_append(dm_args, "-machine");
>      switch (b_info->type) {
>      case LIBXL_DOMAIN_TYPE_PV:
> @@ -616,8 +640,8 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, b_info->extra_pv[i]);
>          break;
>      case LIBXL_DOMAIN_TYPE_HVM:
> -        if (b_info->qemu_machine)
> -            flexarray_append(dm_args, GCSPRINTF("%s,accel=xen",
b_info->qemu_machine));
> +        if (machine)
> +            flexarray_append(dm_args, GCSPRINTF("%s,accel=xen",
machine));
>          else
>              flexarray_append(dm_args, "xenfv");
>          for (i = 0; b_info->extra_hvm &&
b_info->extra_hvm[i] != NULL; i++)
> -- 
> Anthony PERARD
>

Il 29/11/2013 13:31, Stefano Stabellini ha scritto:
> On Fri, 22 Nov 2013, Anthony PERARD wrote:
>> This should result in QEMU *not* adding the xen-platform device.
>>
>> Since QEMU 1.6, this can be achieved by using a different qemu machine.
>> The one used by libxl is "xenfv", but using QEMU >=1.6
with "-machine
>> pc,accel=xen" works as well with only one difference compared to
>> "xenfv", there is no xen-platform device.
>>
>> One more things, if "qemu_machine_override" is set by the
user, then we
>> check if it''s necessary to add the xen-platform device.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>   tools/libxl/libxl_dm.c | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 3d4a913..33caa2b 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -390,6 +390,7 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>>       const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
>>       const libxl_sdl_info *sdl = dm_sdl(guest_config);
>>       const char *keymap = dm_keymap(guest_config);
>> +    const char *machine = b_info->qemu_machine;
>>       flexarray_t *dm_args;
>>       int i;
>>       uint64_t ram_size;
>> @@ -608,6 +609,29 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>>       }
>>       for (i = 0; b_info->extra && b_info->extra[i] !=
NULL; i++)
>>           flexarray_append(dm_args, b_info->extra[i]);
>> +
>> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>> +        if (libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
>> +            if (b_info->qemu_machine) {
>> +                /* Check the qemu machine asked for, it can be
"xenfv" which
>> +                 * will add xen-platform, or it can be "pc"
or "pc-*" which
>> +                 * in this case will need to add the device here.
Anything
>> +                 * else is either a mistake or a machine not supported
by
>> +                 * xen. */
> in that case it would be appropriate to print a warning or an error
This version is old.
There is a v2 of that patch already on git:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=2bc047635b51abd41c917aa2b813211ee0de2c38
And also doc patch:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=80507420f96b06d68a5b23c18998486330b49657
>
>
>> +                if (!strncmp("pc", b_info->qemu_machine,
2)) {
>> +                    flexarray_vappend(dm_args, "-device",
"xen-platform");
>> +                }
>> +            }
>> +        } else {
>> +            /* Switching here to the machine "pc" which does
not add
>> +             * the xen-platform device instead of the default
"xenfv" machine.
>> +             */
>> +            if (!b_info->qemu_machine) {
>> +                machine = "pc";
>> +            }
>> +        }
>> +    }
>> +
>>       flexarray_append(dm_args, "-machine");
>>       switch (b_info->type) {
>>       case LIBXL_DOMAIN_TYPE_PV:
>> @@ -616,8 +640,8 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>>               flexarray_append(dm_args, b_info->extra_pv[i]);
>>           break;
>>       case LIBXL_DOMAIN_TYPE_HVM:
>> -        if (b_info->qemu_machine)
>> -            flexarray_append(dm_args,
GCSPRINTF("%s,accel=xen", b_info->qemu_machine));
>> +        if (machine)
>> +            flexarray_append(dm_args,
GCSPRINTF("%s,accel=xen", machine));
>>           else
>>               flexarray_append(dm_args, "xenfv");
>>           for (i = 0; b_info->extra_hvm &&
b_info->extra_hvm[i] != NULL; i++)
>> -- 
>> Anthony PERARD
>>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Il 25/11/2013 12:11, Anthony PERARD ha scritto:
> On Mon, Nov 25, 2013 at 10:04:59AM +0100, Fabio Fantoni wrote:
>> Il 22/11/2013 17:54, Anthony PERARD ha scritto:
>>> On Fri, Nov 22, 2013 at 04:49:06PM +0100, Fabio Fantoni wrote:
>>>> Il 22/11/2013 16:13, Anthony PERARD ha scritto:
>>>>> Hi,
>>>>>
>>>>> Here is a little series that attempt to fix the issue
regarding
>>>>> xen_platform_pci=0 not been handled.
>>>>>
>>>>> There are two patches, the first one adds an option to
specifies the QEMU
>>>>> machine that a user wants and we handle the
xen_platform_pci=0 case using the
>>>>> new option.
>>>>>
>>>>> The new options "qemu_machine_override" will help
if one want to try a q35
>>>>> based device model. Otherwise, it will be used by libxl to
switch to the "pc"
>>>>> machine instead of the "xenfv" one when
necessary, as the only difference
>>>>> between both (since QEMU 1.6) is the presence of the
xen-platform pci device.
>> About q35 in theory should be missing only the implementation in
hvmloader,
>> is there a draft somewhere to try or to should be made?
> No, sorry, no draft. In hvmloader, there is an easy change, comment the
> assert that prevent hvmloader from starting, but that only the
> beginning.
>
Thanks for reply.

Today I did the first tests with q35 on xen.
I found that disks/cdrom not works with q35 and old qemu paramters used 
by libxl.
With new qemu parameters works and works also with sata instead ide.
Probably next week I''ll do libxl patches for initial draft of q35 
support, new qemu parameters for disk/cdrom using upstream qemu and sata 
support.

I tried to boot Saucy hvm DomU with q35 and sata but qemu crash on 
kernel loading, tried with both with and without xen-platform.
On attachment the serial kernel logs of both cases.
Can you take a look?

Thanks for any reply and sorry for my bad english.



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

Friday, November 29, 2013, 5:06:20 PM, you wrote:
> Il 25/11/2013 12:11, Anthony PERARD ha scritto:
>> On Mon, Nov 25, 2013 at 10:04:59AM +0100, Fabio Fantoni wrote:
>>> Il 22/11/2013 17:54, Anthony PERARD ha scritto:
>>>> On Fri, Nov 22, 2013 at 04:49:06PM +0100, Fabio Fantoni wrote:
>>>>> Il 22/11/2013 16:13, Anthony PERARD ha scritto:
>>>>>> Hi,
>>>>>>
>>>>>> Here is a little series that attempt to fix the issue
regarding
>>>>>> xen_platform_pci=0 not been handled.
>>>>>>
>>>>>> There are two patches, the first one adds an option to
specifies the QEMU
>>>>>> machine that a user wants and we handle the
xen_platform_pci=0 case using the
>>>>>> new option.
>>>>>>
>>>>>> The new options "qemu_machine_override" will
help if one want to try a q35
>>>>>> based device model. Otherwise, it will be used by libxl
to switch to the "pc"
>>>>>> machine instead of the "xenfv" one when
necessary, as the only difference
>>>>>> between both (since QEMU 1.6) is the presence of the
xen-platform pci device.
>>> About q35 in theory should be missing only the implementation in
hvmloader,
>>> is there a draft somewhere to try or to should be made?
>> No, sorry, no draft. In hvmloader, there is an easy change, comment the
>> assert that prevent hvmloader from starting, but that only the
>> beginning.
>>
> Thanks for reply.
> Today I did the first tests with q35 on xen.
> I found that disks/cdrom not works with q35 and old qemu paramters used 
> by libxl.
> With new qemu parameters works and works also with sata instead ide.
> Probably next week I''ll do libxl patches for initial draft of q35 
> support, new qemu parameters for disk/cdrom using upstream qemu and sata 
> support.
> I tried to boot Saucy hvm DomU with q35 and sata but qemu crash on 
> kernel loading, tried with both with and without xen-platform.
> On attachment the serial kernel logs of both cases.
> Can you take a look?
The logs are of the "xen-platform-pci=0" case i guess ?
Was your kernel patched with Konrad''s draft patch ? (the oops is the
same)
http://www.gossamer-threads.com/lists/xen/devel/308017#308017


Would be nice if the Q35 model works without too much effort :-)

> Thanks for any reply and sorry for my bad english.