thr3ads.net - Xen devel - [PATCH] qemu-stubdom: prevent useless medium change [Nov 2012]

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

Roger Pau Monne

2012-Nov-13 16:02 UTC

[PATCH] qemu-stubdom: prevent useless medium change

qemu-stubdom was stripping the prefix from the "params" xenstore
key in xenstore_parse_domain_config, which was then saved stripped in
a variable. In xenstore_process_event we compare the "param" from
xenstore (not stripped) with the stripped "param" saved in the
variable, which leads to a medium change (even if there isn't any),
since we are comparing something like aio:/path/to/file with
/path/to/file. This only happens one time, since
xenstore_parse_domain_config is the only place where we strip the
prefix. The result of this bug is the following:

xs_read_watch() -> /local/domain/0/backend/qdisk/19/5632/params hdc
close(7)
close blk: backend=/local/domain/0/backend/qdisk/19/5632
node=/local/domain/19/device/vbd/5632
(XEN) HVM18: HVM Loader
(XEN) HVM18: Detected Xen v4.3-unstable
(XEN) HVM18: Xenbus rings @0xfeffc000, event channel 4
(XEN) HVM18: System requested ROMBIOS
(XEN) HVM18: CPU speed is 2400 MHz
(XEN) irq.c:270: Dom18 PCI link 0 changed 0 -> 5
(XEN) HVM18: PCI-ISA link 0 routed to IRQ5
(XEN) irq.c:270: Dom18 PCI link 1 changed 0 -> 10
(XEN) HVM18: PCI-ISA link 1 routed to IRQ10
(XEN) irq.c:270: Dom18 PCI link 2 changed 0 -> 11
(XEN) HVM18: PCI-ISA link 2 routed to IRQ11
(XEN) irq.c:270: Dom18 PCI link 3 changed 0 -> 5
(XEN) HVM18: PCI-ISA link 3 routed to IRQ5
(XEN) HVM18: pci dev 01:3 INTA->IRQ10
(XEN) HVM18: pci dev 03:0 INTA->IRQ5
(XEN) HVM18: pci dev 04:0 INTA->IRQ5
(XEN) HVM18: pci dev 02:0 bar 10 size lx: 02000000
(XEN) HVM18: pci dev 03:0 bar 14 size lx: 01000000
(XEN) HVM18: pci dev 02:0 bar 14 size lx: 00001000
(XEN) HVM18: pci dev 03:0 bar 10 size lx: 00000100
(XEN) HVM18: pci dev 04:0 bar 10 size lx: 00000100
(XEN) HVM18: pci dev 04:0 bar 14 size lx: 00000100
(XEN) HVM18: pci dev 01:1 bar 20 size lx: 00000010
(XEN) HVM18: Multiprocessor initialisation:
(XEN) HVM18:  - CPU0 ... 36-bit phys ... fixed MTRRs ... var MTRRs [2/8] ...
done.
(XEN) HVM18:  - CPU1 ... 36-bit phys ... fixed MTRRs ... var MTRRs [2/8] ...
done.
(XEN) HVM18: Testing HVM environment:
(XEN) HVM18:  - REP INSB across page boundaries ... passed
(XEN) HVM18:  - GS base MSRs and SWAPGS ... passed
(XEN) HVM18: Passed 2 of 2 tests
(XEN) HVM18: Writing SMBIOS tables ...
(XEN) HVM18: Loading ROMBIOS ...
(XEN) HVM18: 9660 bytes of ROMBIOS high-memory extensions:
(XEN) HVM18:   Relocating to 0xfc001000-0xfc0035bc ... done
(XEN) HVM18: Creating MP tables ...
(XEN) HVM18: Loading Cirrus VGABIOS ...
(XEN) HVM18: Loading PCI Option ROM ...
(XEN) HVM18:  - Manufacturer: http://ipxe.org
(XEN) HVM18:  - Product name: iPXE
(XEN) HVM18: Option ROMs:
(XEN) HVM18:  c0000-c8fff: VGA BIOS
(XEN) HVM18:  c9000-d8fff: Etherboot ROM
(XEN) HVM18: Loading ACPI ...
(XEN) HVM18: vm86 TSS at fc00f680
(XEN) HVM18: BIOS map:
(XEN) HVM18:  f0000-fffff: Main BIOS
(XEN) HVM18: E820 table:
(XEN) HVM18:  [00]: 00000000:00000000 - 00000000:0009e000: RAM
(XEN) HVM18:  [01]: 00000000:0009e000 - 00000000:000a0000: RESERVED
(XEN) HVM18:  HOLE: 00000000:000a0000 - 00000000:000e0000
(XEN) HVM18:  [02]: 00000000:000e0000 - 00000000:00100000: RESERVED
(XEN) HVM18:  [03]: 00000000:00100000 - 00000000:3f800000: RAM
(XEN) HVM18:  HOLE: 00000000:3f800000 - 00000000:fc000000
(XEN) HVM18:  [04]: 00000000:fc000000 - 00000001:00000000: RESERVED
(XEN) HVM18: Invoking ROMBIOS ...
(XEN) HVM18: $Revision: 1.221 $ $Date: 2008/12/07 17:32:29 $
(XEN) stdvga.c:147:d18 entering stdvga and caching modes
(XEN) HVM18: VGABios $Id: vgabios.c,v 1.67 2008/01/27 09:44:12 vruppert Exp $
(XEN) HVM18: Bochs BIOS - build: 06/23/99
(XEN) HVM18: $Revision: 1.221 $ $Date: 2008/12/07 17:32:29 $
(XEN) HVM18: Options: apmbios pcibios eltorito PMM
(XEN) HVM18:
(XEN) HVM18: ata0-0: PCHS=16383/16/63 translation=lba LCHS=1024/255/63
(XEN) HVM18: ata0 master: QEMU HARDDISK ATA-7 Hard-Disk (10240 MBytes)
(XEN) HVM18: IDE time out
(XEN) HVM18: ata1 master: QEMU DVD-ROM ATAPI-4 CD-Rom/DVD-Rom
(XEN) HVM18: IDE time out
(XEN) HVM18:
(XEN) HVM18:
(XEN) HVM18:
(XEN) HVM18: Press F12 for boot menu.
(XEN) HVM18:
(XEN) HVM18: Booting from CD-Rom...
(XEN) HVM18: ata_is_ready returned 1
(XEN) HVM18: CDROM boot failure code : 0003
(XEN) HVM18: Boot from CD-Rom failed: could not read the boot disk
(XEN) HVM18:
(XEN) HVM18:
(XEN) HVM18: No bootable device.
(XEN) HVM18: Powering off in 30 seconds.
******************* BLKFRONT for /local/domain/19/device/vbd/5632 **********

backend at /local/domain/0/backend/qdisk/19/5632
Failed to read
/local/domain/0/backend/qdisk/19/5632/feature-flush-cache.
284420 sectors of 512 bytes
**************************
blk_open(/local/domain/19/device/vbd/5632) -> 7

As seen in this trace, the medium change happens just when the
guest is booting, which leads to the guest not being able to boot
because the BIOS is not able to access the device.

This is a regression from Xen 4.1, which is able to boot from "file:/"
based backends when using stubdomains.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Indentation in qemu-traditional is not uniform, so I've used 4 spaces
as tabs and tried to indent in the same level.
---
 xenstore.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index 1857160..d3a4588 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -614,6 +614,12 @@ void xenstore_parse_domain_config(int hvm_domid)
         if (pasprintf(&danger_buf, "%s/device/vbd/%s",
danger_path, e_danger[i]) == -1)
             continue;
 	if (bdrv_open2(bs, danger_buf, BDRV_O_CACHE_WB /* snapshot and write-back */,
&bdrv_raw) == 0) {
+        if (pasprintf(&buf, "%s/params", bpath) == -1)
+                continue;
+        free(params);
+        params = xs_read(xsh, XBT_NULL, buf, &len);
+        if (params == NULL)
+            continue;
 	    pstrcpy(bs->filename, sizeof(bs->filename), params);
 	}
 #else
@@ -667,11 +673,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 
 	drives_table[nb_drives].bdrv = bs;
 	drives_table[nb_drives].used = 1;
-#ifdef CONFIG_STUBDOM
-    media_filename[nb_drives] = strdup(danger_buf);
-#else
     media_filename[nb_drives] = strdup(bs->filename);
-#endif
 	nb_drives++;
 
     }
-- 
1.7.7.5 (Apple Git-26)


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

Ian Jackson

2012-Nov-16 16:18 UTC

head link

Re: [PATCH] qemu-stubdom: prevent useless medium change

Roger Pau Monne writes ("[Xen-devel] [PATCH] qemu-stubdom: prevent useless
medium change"):> qemu-stubdom was stripping the prefix from the "params" xenstore
> key in xenstore_parse_domain_config, which was then saved stripped in
> a variable. In xenstore_process_event we compare the "param" from
> xenstore (not stripped) with the stripped "param" saved in the
> variable, which leads to a medium change (even if there isn''t
any),
> since we are comparing something like aio:/path/to/file with
> /path/to/file. This only happens one time, since
> xenstore_parse_domain_config is the only place where we strip the
> prefix. The result of this bug is the following:
I have been thinking about this.

The reason I''m reluctant to apply this patch is that I''m
worried it
might cause some non-stubdom-related breakage.  I know it feels EBW,
but perhaps the answer is _more_ #ifdef STUBDOM rather than less ?

Or do you think I should just read the code closely enough to
understand it and your patch ?  I suspect it''s a can of worms...

Ian.

Roger Pau Monné

2012-Dec-03 16:27 UTC

head link

Re: [PATCH] qemu-stubdom: prevent useless medium change

On 16/11/12 17:18, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH] qemu-stubdom: prevent
useless medium change"):
>> qemu-stubdom was stripping the prefix from the "params"
xenstore
>> key in xenstore_parse_domain_config, which was then saved stripped in
>> a variable. In xenstore_process_event we compare the "param"
from
>> xenstore (not stripped) with the stripped "param" saved in
the
>> variable, which leads to a medium change (even if there isn''t
any),
>> since we are comparing something like aio:/path/to/file with
>> /path/to/file. This only happens one time, since
>> xenstore_parse_domain_config is the only place where we strip the
>> prefix. The result of this bug is the following:
> 
> I have been thinking about this.
> 
> The reason I''m reluctant to apply this patch is that I''m
worried it
> might cause some non-stubdom-related breakage.  I know it feels EBW,
> but perhaps the answer is _more_ #ifdef STUBDOM rather than less ?
> 
> Or do you think I should just read the code closely enough to
> understand it and your patch ?  I suspect it''s a can of worms...
Yes, it''s a can of worms indeed.

The non-stubdom path is not modified, and the code changes (1st block of
the patch) are contained inside a #ifdef STUBDOM (which is not seen on
the patch itself, because it''s already there).

Thanks, Roger.

Ian Jackson

2012-Dec-06 12:37 UTC

head link

Re: [PATCH] qemu-stubdom: prevent useless medium change [and 1 more messages]

Roger Pau Monne writes ("[Xen-devel] [PATCH] qemu-stubdom: prevent useless
medium change"):> qemu-stubdom was stripping the prefix from the "params" xenstore
> key in xenstore_parse_domain_config, which was then saved stripped in
> a variable. In xenstore_process_event we compare the "param" from
> xenstore (not stripped) with the stripped "param" saved in the
> variable, which leads to a medium change (even if there isn''t
any),
> since we are comparing something like aio:/path/to/file with
> /path/to/file. This only happens one time, since
> xenstore_parse_domain_config is the only place where we strip the
> prefix. The result of this bug is the following:
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH] qemu-stubdom: prevent
useless medium change"):> Yes, it''s a can of worms indeed.
> 
> The non-stubdom path is not modified, and the code changes (1st block of
> the patch) are contained inside a #ifdef STUBDOM (which is not seen on
> the patch itself, because it''s already there).
Aha!  Indeed.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

This needs to go into 4.2 surely ?

Ian.

Roger Pau Monné

2012-Dec-07 09:37 UTC

head link

Re: [PATCH] qemu-stubdom: prevent useless medium change [and 1 more messages]

On 06/12/12 13:37, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH] qemu-stubdom: prevent
useless medium change"):
>> qemu-stubdom was stripping the prefix from the "params"
xenstore
>> key in xenstore_parse_domain_config, which was then saved stripped in
>> a variable. In xenstore_process_event we compare the "param"
from
>> xenstore (not stripped) with the stripped "param" saved in
the
>> variable, which leads to a medium change (even if there isn''t
any),
>> since we are comparing something like aio:/path/to/file with
>> /path/to/file. This only happens one time, since
>> xenstore_parse_domain_config is the only place where we strip the
>> prefix. The result of this bug is the following:
> 
> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH] qemu-stubdom: prevent
useless medium change"):
>> Yes, it''s a can of worms indeed.
>>
>> The non-stubdom path is not modified, and the code changes (1st block
of
>> the patch) are contained inside a #ifdef STUBDOM (which is not seen on
>> the patch itself, because it''s already there).
> 
> Aha!  Indeed.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> This needs to go into 4.2 surely ?
Thanks, and yes, it should be backported to 4.2 stable.
> Ian.
>

Maybe Matching Threads

Search for more maybe matching threads

Xen devel - Nov 2012 - [PATCH] qemu-stubdom: prevent useless medium change

[PATCH] qemu-stubdom: prevent useless medium change

Re: [PATCH] qemu-stubdom: prevent useless medium change

Re: [PATCH] qemu-stubdom: prevent useless medium change

Re: [PATCH] qemu-stubdom: prevent useless medium change [and 1 more messages]

Re: [PATCH] qemu-stubdom: prevent useless medium change [and 1 more messages]

Maybe Matching Threads