All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] libxl: relax readonly check introduced by XSA-142 fix
@ 2015-11-11 17:15 Jim Fehlig
  2015-11-12 12:01 ` Stefano Stabellini
  0 siblings, 1 reply; 7+ messages in thread
From: Jim Fehlig @ 2015-11-11 17:15 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell, Stefano Stabellini

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

Hi All,

Apologies for only noticing the fix for XSA-142 as it starting flowing to our
various downstreams. The current fix seems like quite a big hammer IMO. qemu
doesn't support readonly IDE/SATA disks

# /usr/lib/xen/bin/qemu-system-i386 -drive
file=/tmp/disk.raw,if=ide,media=disk,format=raw,readonly=on
qemu-system-i386: Can't use a read-only drive

But it does support readonly SCSI disks

# /usr/lib/xen/bin/qemu-system-i386 -drive
file=/tmp/disk.raw,if=scsi,media=disk,format=raw,readonly=on
[ok]

Inside a guest using such a disk, the SCSI kernel driver sees write protect on

[   7.339232] sd 2:0:1:0: [sdb] Write Protect is on

Also, PV drivers support readonly, but the patch rejects such configuration even
when PV drivers (vdev=xvd*) have been explicitly specified and creation of an
emulated twin is skipped.

The attached follow-up loosens the restriction to reject readonly when creating
and emulated IDE disk, but allows it when the backend is known to support readonly.

Regards,
Jim

[-- Attachment #2: relax-xsa142-readonly-check.patch --]
[-- Type: text/x-diff, Size: 4731 bytes --]

>From 3d58f93b58ab544b9ee168b7bbaf59de1c5532ce Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@suse.com>
Date: Tue, 10 Nov 2015 11:33:44 -0700
Subject: [PATCH] libxl: relax readonly check introduced by XSA-142 fix

The fix for XSA-142 is quite a big hammer, rejecting readonly
disk configuration even when the requested backend is known to
support readonly. While it is true that qemu doesn't support
readonly for emulated IDE disks

$ /usr/lib/xen/bin/qemu-system-i386
 -drive file=/tmp/disk.raw,if=ide,media=disk,format=raw,readonly=on
qemu-system-i386: Can't use a read-only drive

It does support readonly SCSI disks

$ /usr/lib/xen/bin/qemu-system-i386
 -drive file=/tmp/disk.raw,if=scsi,media=disk,format=raw,readonly=on
[ok]

Inside a guest using such a disk, the SCSI kernel driver sees write
protect on

[   7.339232] sd 2:0:1:0: [sdb] Write Protect is on

Also, PV drivers support readonly, but the patch rejects such
configuration even when PV drivers (vdev=xvd*) have been explicitly
specified and creation of an emulated twin is skiped.

This follow-up patch loosens the restriction to reject readonly when
creating and emulated IDE disk, but allows it when the backend is
known to support readonly.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 tools/libxl/libxl_dm.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 9c9eaa3..ccfc827 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1152,12 +1152,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                         (gc, "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
                          disks[i].pdev_path, disk, disks[i].readwrite ? "off" : "on", format, dev_number);
             } else {
-                if (!disks[i].readwrite) {
-                    LOG(ERROR,
-                        "qemu-xen doesn't support read-only disk drivers");
-                    return ERROR_INVAL;
-                }
-
                 if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
                     LOG(WARN, "cannot support"" empty disk format for %s",
                         disks[i].vdev);
@@ -1185,29 +1179,34 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                  * For other disks we translate devices 0..3 into
                  * hd[a-d] and ignore the rest.
                  */
-                if (strncmp(disks[i].vdev, "sd", 2) == 0)
+                if (strncmp(disks[i].vdev, "sd", 2) == 0) {
                     drive = libxl__sprintf
-                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
-                         pdev_path, disk, format);
-                else if (strncmp(disks[i].vdev, "xvd", 3) == 0)
+                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
+                         pdev_path, disk, format, disks[i].readwrite ? "off" : "on");
+                } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) {
                     /*
                      * Do not add any emulated disk when PV disk are
                      * explicitly asked for.
                      */
                     continue;
-                else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
+                } else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
                     flexarray_vappend(dm_args, "-drive",
-                        GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
-                        pdev_path, disk, format),
+                        GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,readonly=%s,cache=writeback",
+                                  pdev_path, disk, format, disks[i].readwrite ? "off" : "on"),
                         "-device", GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
                         disk, disk), NULL);
                     continue;
-                } else if (disk < 4)
+                } else if (disk < 4) {
+                    if (!disks[i].readwrite) {
+                        LOG(ERROR, "qemu-xen doesn't support read-only IDE disk drivers");
+                        return ERROR_INVAL;
+                    }
                     drive = libxl__sprintf
                         (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
                          pdev_path, disk, format);
-                else
+                } else {
                     continue; /* Do not emulate this disk */
+                }
             }
 
             flexarray_append(dm_args, "-drive");
-- 
1.8.0.1


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix
  2015-11-11 17:15 [RFC] libxl: relax readonly check introduced by XSA-142 fix Jim Fehlig
@ 2015-11-12 12:01 ` Stefano Stabellini
  2015-11-12 15:45   ` Jim Fehlig
  0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2015-11-12 12:01 UTC (permalink / raw)
  To: Jim Fehlig
  Cc: Ian Jackson, Stefano Stabellini, Wei Liu, Ian Campbell, xen-devel

On Wed, 11 Nov 2015, Jim Fehlig wrote:

> Hi All,
> 
> Apologies for only noticing the fix for XSA-142 as it starting flowing to our
> various downstreams. The current fix seems like quite a big hammer IMO. qemu
> doesn't support readonly IDE/SATA disks
> 
> # /usr/lib/xen/bin/qemu-system-i386 -drive
> file=/tmp/disk.raw,if=ide,media=disk,format=raw,readonly=on
> qemu-system-i386: Can't use a read-only drive
> 
> But it does support readonly SCSI disks
> 
> # /usr/lib/xen/bin/qemu-system-i386 -drive
> file=/tmp/disk.raw,if=scsi,media=disk,format=raw,readonly=on
> [ok]
> 
> Inside a guest using such a disk, the SCSI kernel driver sees write protect on
> 
> [   7.339232] sd 2:0:1:0: [sdb] Write Protect is on
> 
> Also, PV drivers support readonly, but the patch rejects such configuration even
> when PV drivers (vdev=xvd*) have been explicitly specified and creation of an
> emulated twin is skipped.
> 
> The attached follow-up loosens the restriction to reject readonly when creating
> and emulated IDE disk, but allows it when the backend is known to support readonly.
> 
> Regards,
> Jim

Hi Jim,

thanks for the patch, I think is good.  Just one question below:


> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 9c9eaa3..ccfc827 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1152,12 +1152,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                          (gc, "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
>                           disks[i].pdev_path, disk, disks[i].readwrite ? "off" : "on", format, dev_number);
>              } else {
> -                if (!disks[i].readwrite) {
> -                    LOG(ERROR,
> -                        "qemu-xen doesn't support read-only disk drivers");
> -                    return ERROR_INVAL;
> -                }
> -
>                  if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>                      LOG(WARN, "cannot support"" empty disk format for %s",
>                          disks[i].vdev);
> @@ -1185,29 +1179,34 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                   * For other disks we translate devices 0..3 into
>                   * hd[a-d] and ignore the rest.
>                   */
> -                if (strncmp(disks[i].vdev, "sd", 2) == 0)
> +                if (strncmp(disks[i].vdev, "sd", 2) == 0) {
>                      drive = libxl__sprintf
> -                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
> -                         pdev_path, disk, format);
> -                else if (strncmp(disks[i].vdev, "xvd", 3) == 0)
> +                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
> +                         pdev_path, disk, format, disks[i].readwrite ? "off" : "on");
> +                } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) {
>                      /*
>                       * Do not add any emulated disk when PV disk are
>                       * explicitly asked for.
>                       */
>                      continue;
> -                else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
> +                } else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
>                      flexarray_vappend(dm_args, "-drive",
> -                        GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
> -                        pdev_path, disk, format),
> +                        GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,readonly=%s,cache=writeback",
> +                                  pdev_path, disk, format, disks[i].readwrite ? "off" : "on"),
>                          "-device", GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
>                          disk, disk), NULL);

The commit message doesn't say anything about AHCI. Are AHCI disks
actually emulated correctly by QEMU with readonly=on?


>                      continue;
> -                } else if (disk < 4)
> +                } else if (disk < 4) {
> +                    if (!disks[i].readwrite) {
> +                        LOG(ERROR, "qemu-xen doesn't support read-only IDE disk drivers");
> +                        return ERROR_INVAL;
> +                    }
>                      drive = libxl__sprintf
>                          (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
>                           pdev_path, disk, format);
> -                else
> +                } else {
>                      continue; /* Do not emulate this disk */
> +                }

I don't think that libxl CODING_STYLE requires brackets for one statement
blocks. But I won't enforce it.


>              }
>  
>              flexarray_append(dm_args, "-drive");
> -- 
> 1.8.0.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix
  2015-11-12 12:01 ` Stefano Stabellini
@ 2015-11-12 15:45   ` Jim Fehlig
  2015-11-12 16:13     ` Ian Jackson
  2015-11-12 16:26     ` Ian Campbell
  0 siblings, 2 replies; 7+ messages in thread
From: Jim Fehlig @ 2015-11-12 15:45 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: Ian Jackson, Wei Liu, Ian Campbell, xen-devel

Stefano Stabellini wrote:
> On Wed, 11 Nov 2015, Jim Fehlig wrote:
> 
>> Hi All,
>>
>> Apologies for only noticing the fix for XSA-142 as it starting flowing to our
>> various downstreams. The current fix seems like quite a big hammer IMO. qemu
>> doesn't support readonly IDE/SATA disks
>>
>> # /usr/lib/xen/bin/qemu-system-i386 -drive
>> file=/tmp/disk.raw,if=ide,media=disk,format=raw,readonly=on
>> qemu-system-i386: Can't use a read-only drive
>>
>> But it does support readonly SCSI disks
>>
>> # /usr/lib/xen/bin/qemu-system-i386 -drive
>> file=/tmp/disk.raw,if=scsi,media=disk,format=raw,readonly=on
>> [ok]
>>
>> Inside a guest using such a disk, the SCSI kernel driver sees write protect on
>>
>> [   7.339232] sd 2:0:1:0: [sdb] Write Protect is on
>>
>> Also, PV drivers support readonly, but the patch rejects such configuration even
>> when PV drivers (vdev=xvd*) have been explicitly specified and creation of an
>> emulated twin is skipped.
>>
>> The attached follow-up loosens the restriction to reject readonly when creating
>> and emulated IDE disk, but allows it when the backend is known to support readonly.
>>
>> Regards,
>> Jim
> 
> Hi Jim,
> 
> thanks for the patch, I think is good.  Just one question below:
> 
> 
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 9c9eaa3..ccfc827 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -1152,12 +1152,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>>                          (gc, "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
>>                           disks[i].pdev_path, disk, disks[i].readwrite ? "off" : "on", format, dev_number);
>>              } else {
>> -                if (!disks[i].readwrite) {
>> -                    LOG(ERROR,
>> -                        "qemu-xen doesn't support read-only disk drivers");
>> -                    return ERROR_INVAL;
>> -                }
>> -
>>                  if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>>                      LOG(WARN, "cannot support"" empty disk format for %s",
>>                          disks[i].vdev);
>> @@ -1185,29 +1179,34 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>>                   * For other disks we translate devices 0..3 into
>>                   * hd[a-d] and ignore the rest.
>>                   */
>> -                if (strncmp(disks[i].vdev, "sd", 2) == 0)
>> +                if (strncmp(disks[i].vdev, "sd", 2) == 0) {
>>                      drive = libxl__sprintf
>> -                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
>> -                         pdev_path, disk, format);
>> -                else if (strncmp(disks[i].vdev, "xvd", 3) == 0)
>> +                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
>> +                         pdev_path, disk, format, disks[i].readwrite ? "off" : "on");
>> +                } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) {
>>                      /*
>>                       * Do not add any emulated disk when PV disk are
>>                       * explicitly asked for.
>>                       */
>>                      continue;
>> -                else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
>> +                } else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) {
>>                      flexarray_vappend(dm_args, "-drive",
>> -                        GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
>> -                        pdev_path, disk, format),
>> +                        GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,readonly=%s,cache=writeback",
>> +                                  pdev_path, disk, format, disks[i].readwrite ? "off" : "on"),
>>                          "-device", GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
>>                          disk, disk), NULL);
> 
> The commit message doesn't say anything about AHCI. Are AHCI disks
> actually emulated correctly by QEMU with readonly=on?

I just double checked, and good thing since AHCI + readonly is another rejected
combination

/usr/lib/xen/bin/qemu-system-i386 -device ahci,id=ahci0 \
 -drive file=/tmp/disk.raw,if=none,id=ahcidisk-0,format=raw,readonly=on \
 -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0
qemu-system-i386: -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0: Can't use
a read-only drive

So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and ammend
the commit message in V2.

> 
> 
>>                      continue;
>> -                } else if (disk < 4)
>> +                } else if (disk < 4) {
>> +                    if (!disks[i].readwrite) {
>> +                        LOG(ERROR, "qemu-xen doesn't support read-only IDE disk drivers");
>> +                        return ERROR_INVAL;
>> +                    }
>>                      drive = libxl__sprintf
>>                          (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
>>                           pdev_path, disk, format);
>> -                else
>> +                } else {
>>                      continue; /* Do not emulate this disk */
>> +                }
> 
> I don't think that libxl CODING_STYLE requires brackets for one statement
> blocks. But I won't enforce it.

I can remove the unneeded brackets if desired. That habit comes from working on
projects where all if/else blocks are required to have brackets when any one of
the blocks requires brackets.

Regards,
Jim

> 
> 
>>              }
>>  
>>              flexarray_append(dm_args, "-drive");
>> -- 
>> 1.8.0.1
>>
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix
  2015-11-12 15:45   ` Jim Fehlig
@ 2015-11-12 16:13     ` Ian Jackson
  2015-11-12 16:26     ` Ian Campbell
  1 sibling, 0 replies; 7+ messages in thread
From: Ian Jackson @ 2015-11-12 16:13 UTC (permalink / raw)
  To: Jim Fehlig; +Cc: xen-devel, Wei Liu, Ian Campbell, Stefano Stabellini

Jim Fehlig writes ("Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix"):
> Stefano Stabellini wrote:
> > I don't think that libxl CODING_STYLE requires brackets for one statement
> > blocks. But I won't enforce it.

Thanks for all the work on this, Jim, and Stefano.  It looks good to
me.  The only thing I found to comment on was this tiny nit:

> I can remove the unneeded brackets if desired. That habit comes from
> working on projects where all if/else blocks are required to have
> brackets when any one of the blocks requires brackets.

This isn't in CODING_STYLE but it probably should be.  I would prefer
it with the brackets.  (But I'll accept the patch either way.)

Ian.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix
  2015-11-12 15:45   ` Jim Fehlig
  2015-11-12 16:13     ` Ian Jackson
@ 2015-11-12 16:26     ` Ian Campbell
  2015-11-12 17:53       ` Jim Fehlig
  1 sibling, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2015-11-12 16:26 UTC (permalink / raw)
  To: Jim Fehlig, Stefano Stabellini; +Cc: Ian Jackson, Wei Liu, xen-devel

On Thu, 2015-11-12 at 08:45 -0700, Jim Fehlig wrote:
> 
> > The commit message doesn't say anything about AHCI. Are AHCI disks
> > actually emulated correctly by QEMU with readonly=on?
> 
> I just double checked, and good thing since AHCI + readonly is another
> rejected
> combination
> 
> /usr/lib/xen/bin/qemu-system-i386 -device ahci,id=ahci0 \
>  -drive file=/tmp/disk.raw,if=none,id=ahcidisk-0,format=raw,readonly=on \
>  -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0
> qemu-system-i386: -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0:
> Can't use
> a read-only drive
> 
> So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and
> ammend
> the commit message in V2.

Just to clarify when you say "rejected" and "incompatible" do you mean that
qemu will fail to start if you try, or that it will ignore you and give a
writeable disk?

If, as I think, it will fail, why don't we just always ask and rely on qemu
to reject, instead of trying to whitelist the ones we know work in the
libxl code?

That way as people add r/o to various device types in QEMU it'll Just Work.

Ian.


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix
  2015-11-12 16:26     ` Ian Campbell
@ 2015-11-12 17:53       ` Jim Fehlig
  2015-11-13  9:22         ` Ian Campbell
  0 siblings, 1 reply; 7+ messages in thread
From: Jim Fehlig @ 2015-11-12 17:53 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Ian Jackson, xen-devel, Wei Liu, Stefano Stabellini

Ian Campbell wrote:
> On Thu, 2015-11-12 at 08:45 -0700, Jim Fehlig wrote:
>>  
>>> The commit message doesn't say anything about AHCI. Are AHCI disks
>>> actually emulated correctly by QEMU with readonly=on?
>> I just double checked, and good thing since AHCI + readonly is another
>> rejected
>> combination
>>
>> /usr/lib/xen/bin/qemu-system-i386 -device ahci,id=ahci0 \
>>  -drive file=/tmp/disk.raw,if=none,id=ahcidisk-0,format=raw,readonly=on \
>>  -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0
>> qemu-system-i386: -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0:
>> Can't use
>> a read-only drive
>>
>> So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and
>> ammend
>> the commit message in V2.
> 
> Just to clarify when you say "rejected" and "incompatible" do you mean that
> qemu will fail to start if you try, or that it will ignore you and give a
> writeable disk?

qemu will fail to start.

> 
> If, as I think, it will fail, why don't we just always ask and rely on qemu
> to reject, instead of trying to whitelist the ones we know work in the
> libxl code?

That would be possible, but makes it more difficult to track down why the domain
failed to start. With the check in libxl:

# xl create sles12-hvm.xl
Parsing config from sles12-hvm.xl
libxl: error: libxl_dm.c:1201:libxl__build_device_model_args_new: qemu-xen
doesn't support read-only IDE disk drivers
libxl: error: libxl_dm.c:1891:device_model_spawn_outcome: (null): spawn failed
(rc=-6)
libxl: error: libxl_create.c:1340:domcreate_devmodel_started: device model did
not start: -6

Allowing qemu to fail:
# xl create sles12-hvm.xl
Parsing config from sles12-hvm.xl
libxl: error: libxl_dm.c:1887:device_model_spawn_outcome: domain 14 device
model: spawn failed (rc=-3)
libxl: error: libxl_create.c:1340:domcreate_devmodel_started: device model did
not start: -3
libxl: error: libxl_dm.c:1997:kill_device_model: Device Model already exited

Ok, not so obvious why qemu failed to start. One would need to peek at
/var/log/xen/qemu-dm-sles12-hvm.log:

char device redirected to /dev/pts/3 (label serial0)
qemu-system-i386: Can't use a read-only drive
qemu-system-i386: Device initialization failed.

Regards,
Jim

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] libxl: relax readonly check introduced by XSA-142 fix
  2015-11-12 17:53       ` Jim Fehlig
@ 2015-11-13  9:22         ` Ian Campbell
  0 siblings, 0 replies; 7+ messages in thread
From: Ian Campbell @ 2015-11-13  9:22 UTC (permalink / raw)
  To: Jim Fehlig; +Cc: Ian Jackson, xen-devel, Wei Liu, Stefano Stabellini

On Thu, 2015-11-12 at 10:53 -0700, Jim Fehlig wrote:
> Ian Campbell wrote:
> > On Thu, 2015-11-12 at 08:45 -0700, Jim Fehlig wrote:
> > >  
> > > > The commit message doesn't say anything about AHCI. Are AHCI disks
> > > > actually emulated correctly by QEMU with readonly=on?
> > > I just double checked, and good thing since AHCI + readonly is
> > > another
> > > rejected
> > > combination
> > > 
> > > /usr/lib/xen/bin/qemu-system-i386 -device ahci,id=ahci0 \
> > >  -drive file=/tmp/disk.raw,if=none,id=ahcidisk-
> > > 0,format=raw,readonly=on \
> > >  -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0
> > > qemu-system-i386: -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0:
> > > Can't use
> > > a read-only drive
> > > 
> > > So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this
> > > and
> > > ammend
> > > the commit message in V2.
> > 
> > Just to clarify when you say "rejected" and "incompatible" do you mean
> > that
> > qemu will fail to start if you try, or that it will ignore you and give
> > a
> > writeable disk?
> 
> qemu will fail to start.

OK, that's good, I was a bit worried it might fail open.

> > If, as I think, it will fail, why don't we just always ask and rely on
> > qemu
> > to reject, instead of trying to whitelist the ones we know work in the
> > libxl code?
> 
> That would be possible, but makes it more difficult to track down why the domain
> failed to start.[...]

Indeed.

> libxl: error: libxl_create.c:1340:domcreate_devmodel_started: device model did
> not start: -6

At a minimum this ought to do as the bootloader failed message does and say
"look in /var/log/xen/qemu-dm-sles12-hvm.log for more info". Ideally error
reporting from qemu back to the toolstack would be able to actually report
back what was going on somehow (which I appreciate might be rather
difficult to arrange).

Anyway, none of that is on you and since qemu fails safe if libxl gets it
wrong I don't think it should block this patch.

Ian.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-11-13  9:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-11 17:15 [RFC] libxl: relax readonly check introduced by XSA-142 fix Jim Fehlig
2015-11-12 12:01 ` Stefano Stabellini
2015-11-12 15:45   ` Jim Fehlig
2015-11-12 16:13     ` Ian Jackson
2015-11-12 16:26     ` Ian Campbell
2015-11-12 17:53       ` Jim Fehlig
2015-11-13  9:22         ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.