PV driver compatibility

PATCH In review Ky Srinivasan 4 participants 12 messages Jan 20, 2010 – Jan 26, 2010

Share & tools

Ky Srinivasan

author

2010-01-20 16:02 UTC #0 lore raw

PATCH 1/1 PV driver compatibility

The attached patch fixes what I believe is a typo and permits guests running the latest PV drivers to correctly interact with older back-ends.

Signed-off-by K. Y. Srinivasan <[email protected]>

Make our PV drivers work with older hosts that do not recognize the new PV driver protocol.

Signed-off-by K. Y. Srinivasan <[email protected]>

unmodified_drivers/linux-2.6/platform-pci/platform-pci.c +4 -1

Index: xen-4.0.0-testing/unmodified_drivers/linux-2.6/platform-pci/platform-pci.c
===================================================================
@@ -284,7 +284,10 @@
 
 	if (magic != XEN_IOPORT_MAGIC_VAL) {
 		dev_err(dev, "invalid magic %#x", magic);
-		return -ENODEV;
+		/* 
+		 * Older backend; just return 0 to be compatible.
+		 */
+		return 0;
 	}
 
 	protocol = inb(XEN_IOPORT_PROTOVER);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2010-01-20 16:07 UTC #1 lore raw

Quoted 4 lines · Ky Srinivasan

The attached patch fixes what I believe is a typo and permits guests running the latest PV drivers to correctly interact with older back-ends.
Signed-off-by K. Y. Srinivasan <[email protected]>

Ian,

You introduced the magic port value check, in xen-unstable:19964. Can you ack/nack this please?

-- Keir

Ian Campbell

2010-01-21 20:38 UTC #2 lore raw

Quoted 10 lines · Keir Fraser

Quoted 4 lines · Ky Srinivasan
The attached patch fixes what I believe is a typo and permits guests running the latest PV drivers to correctly interact with older back-ends.
Signed-off-by K. Y. Srinivasan <[email protected]>
Ian,
You introduced the magic port value check, in xen-unstable:19964.

I'm guilty of pretty poor changelogging there, aren't I, I've no idea how the unmodified drivers part of the change relates to the comment :-(

Quoted 1 line

Can you ack/nack this please?

What vintage of older back-ends are we talking about?

What is their behaviour when reading from that port? Can we test for a specific value instead of anything != MAGIC or is there some other way to identify them?

Without some sort of unplugging mechanism we run the risk of having both PV and Emulated disk controllers active, accessing the same virtual disk and with drivers loaded in the guest, which is potentially very dangerous for the user's data. Did those older backends implement some alternative unplugging mechanism we should be trying?

The whole point of this magic check is to ensure we are running on a backend which is new enough to do the unplugging in a safe way, so I think failing to switch to PV and sticking with emulated on such platforms the safe approach.

I'd suggest that this issue should be fixed by backporting the backend support for the safe unplug protocol -- I don't think the patch for such minimal support is that big or risky. Failing that perhaps it could at least be something you need to explicitly ask for if you have somehow verified that there is no danger of the emulated and PV backends trampling each other, e.g. via a frontend module parameter.

Ian.

Keir Fraser

2010-01-21 21:46 UTC #3 lore raw

Quoted 4 lines · Ian Campbell

Quoted 1 line
You introduced the magic port value check, in xen-unstable:19964.
I'm guilty of pretty poor changelogging there, aren't I, I've no idea how the unmodified drivers part of the change relates to the comment :-(

Yes, I wonder if it was even meant to be checked in. Or if it's an accidental merge of two patches both of which you intended to go in.

I'm not sure unconditional unplug, as implemented, is a good idea. I can imagine people with setups in which emulated devices coexist with pv devices. Such a setup breaks if emulated devices all get unplugged when pv drivers load. Presumably in the environment this patch came from (Citrix XenServer) such a situation is disallowed, but I'm not so sure about proscribing it more generally.

Some people do want unplug though, so I think making it a non-default module option is a good idea.

-- Keir

Ian Campbell

2010-01-22 07:31 UTC #4 lore raw

Quoted 9 lines · Keir Fraser

Quoted 4 lines · Ian Campbell
Quoted 1 line
You introduced the magic port value check, in xen-unstable:19964.
I'm guilty of pretty poor changelogging there, aren't I, I've no idea how the unmodified drivers part of the change relates to the comment :-(
Yes, I wonder if it was even meant to be checked in. Or if it's an accidental merge of two patches both of which you intended to go in.

Yes, that's possible. I notice that I defined XEN_IOPORT_LINUX_PRODNUM and XEN_IOPORT_LINUX_DRVVER but then hardcode 0xdead and 0xbeef where they should be used, which suggests I wasn't quite ready to send the patch...

Quoted 7 lines

I'm not sure unconditional unplug, as implemented, is a good idea. I can imagine people with setups in which emulated devices coexist with pv devices. Such a setup breaks if emulated devices all get unplugged when pv drivers load. Presumably in the environment this patch came from (Citrix XenServer) such a situation is disallowed, but I'm not so sure about proscribing it more generally.

The protocol allows for coarse grained selection of which devices to unplug:
6) The drivers write a two-byte bitmask of devices to unplug to IO port 0x10. The defined fields are:

1 -- All IDE disks (not including CD drives)
2 -- All emulated NICs
4 -- All IDE disks except for the primary master (not including CD
drives)

There is scope for extending this to a more explicit bitmask allowing individual devices to be selected, if people are interested in that.

Quoted 6 lines

Some people do want unplug though, so I think making it a non-default module option is a good idea.
-- Keir

Keir Fraser

2010-01-22 08:03 UTC #5 lore raw

Quoted 4 lines · Ian Campbell

Yes, that's possible. I notice that I defined XEN_IOPORT_LINUX_PRODNUM and XEN_IOPORT_LINUX_DRVVER but then hardcode 0xdead and 0xbeef where they should be used, which suggests I wasn't quite ready to send the patch...

Hm, do you want to keep the deadbeef, or pick a nicer value, or remove the patch?

Quoted 4 lines

The protocol allows for coarse grained selection of which devices to unplug:
6) The drivers write a two-byte bitmask of devices to unplug to IO port 0x10. The defined fields are:

Well, the flexibility is nice, but not a feature that driver currently actually uses.

My default action right now will be to add a module option to specify selective or complete flushing, keep the patch, but only fail to load drivers if flush is requested but not possible, otherwise just warn.

-- Keir

Ian Campbell

2010-01-22 08:43 UTC #6 lore raw

Quoted 15 lines · Keir Fraser

Quoted 4 lines · Ian Campbell
Yes, that's possible. I notice that I defined XEN_IOPORT_LINUX_PRODNUM and XEN_IOPORT_LINUX_DRVVER but then hardcode 0xdead and 0xbeef where they should be used, which suggests I wasn't quite ready to send the patch...
Hm, do you want to keep the deadbeef, or pick a nicer value, or remove the patch?
Quoted 4 lines
The protocol allows for coarse grained selection of which devices to unplug:
6) The drivers write a two-byte bitmask of devices to unplug to IO port 0x10. The defined fields are:

Quoted 6 lines

Well, the flexibility is nice, but not a feature that driver currently actually uses.
My default action right now will be to add a module option to specify selective or complete flushing, keep the patch, but only fail to load drivers if flush is requested but not possible, otherwise just warn.

Sounds good, what will the default flush setting be?

Could you swap out the 0xdead and 0xbeef for the defines which are specified right above the function in question while you are there?

Ian.

Keir Fraser

2010-01-22 09:07 UTC #7 lore raw

Quoted 5 lines · Ian Campbell

Quoted 5 lines
Quoted 4 lines
The protocol allows for coarse grained selection of which devices to unplug:
6) The drivers write a two-byte bitmask of devices to unplug to IO port 0x10. The defined fields are:

Quoted 8 lines

Quoted 6 lines
Well, the flexibility is nice, but not a feature that driver currently actually uses.
My default action right now will be to add a module option to specify selective or complete flushing, keep the patch, but only fail to load drivers if flush is requested but not possible, otherwise just warn.
Sounds good, what will the default flush setting be?

None. Those who are switching from emulated to pv should know what they are doing and specify the module option.

Quoted 2 lines

Could you swap out the 0xdead and 0xbeef for the defines which are specified right above the function in question while you are there?

Yep.

-- Keir

Keir Fraser

2010-01-22 10:45 UTC #8 lore raw

In reply to Ian Campbell

Quoted 8 lines · Ian Campbell

Quoted 3 lines
My default action right now will be to add a module option to specify selective or complete flushing, keep the patch, but only fail to load drivers if flush is requested but not possible, otherwise just warn.
Sounds good, what will the default flush setting be?
Could you swap out the 0xdead and 0xbeef for the defines which are specified right above the function in question while you are there?

Proposed patch is attached. It'd be good if someone could at least build-test it -- I can't seem to work out how to build PV-on-HVM drivers any more, against my stock selection of native kernels.

-- Keir

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Pasi Kärkkäinen

2010-01-22 11:16 UTC #9 lore raw

Quoted 6 lines · Keir Fraser

Quoted 4 lines · Ian Campbell
Quoted 3 lines
My default action right now will be to add a module option to specify selective or complete flushing, keep the patch, but only fail to load drivers if flush is requested but not possible, otherwise just warn.

Quoted 9 lines

Quoted 4 lines
Sounds good, what will the default flush setting be?
Could you swap out the 0xdead and 0xbeef for the defines which are specified right above the function in question while you are there?
Proposed patch is attached. It'd be good if someone could at least build-test it -- I can't seem to work out how to build PV-on-HVM drivers any more, against my stock selection of native kernels.

Maybe this blog post helps:

"building unmodified_drivers":
http://wp.colliertech.org/cj/?p=653

That's about the SLES11 2.6.27 kernel.

-- Pasi

Pasi Kärkkäinen

2010-01-26 10:12 UTC #10 lore raw

Quoted 17 lines · Pasi Kärkkäinen

Quoted 6 lines · Keir Fraser
Quoted 4 lines · Ian Campbell
Quoted 3 lines
My default action right now will be to add a module option to specify selective or complete flushing, keep the patch, but only fail to load drivers if flush is requested but not possible, otherwise just warn.
Quoted 9 lines
Quoted 4 lines
Sounds good, what will the default flush setting be?
Could you swap out the 0xdead and 0xbeef for the defines which are specified right above the function in question while you are there?
Proposed patch is attached. It'd be good if someone could at least build-test it -- I can't seem to work out how to build PV-on-HVM drivers any more, against my stock selection of native kernels.

Quoted 7 lines

Maybe this blog post helps:
"building unmodified_drivers":
http://wp.colliertech.org/cj/?p=653
That's about the SLES11 2.6.27 kernel.

Btw against what kernels did you try to build?

-- Pasi

Keir Fraser

2010-01-26 10:19 UTC #11 lore raw

Quoted 7 lines · Pasi Kärkkäinen

Quoted 7 lines
Quoted 4 lines
Proposed patch is attached. It'd be good if someone could at least build-test it -- I can't seem to work out how to build PV-on-HVM drivers any more, against my stock selection of native kernels.
Maybe this blog post helps:

Quoted 7 lines

Quoted 5 lines
"building unmodified_drivers":
http://wp.colliertech.org/cj/?p=653
That's about the SLES11 2.6.27 kernel.
Btw against what kernels did you try to build?

Vanilla 2.6.19 and vanilla 2.6.27.

-- Keir