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 <ksrinivasan@novell.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 20/01/2010 16:02, "Ky Srinivasan" <ksrinivasan@novell.com> wrote:> 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 <ksrinivasan@novell.com>Ian, You introduced the magic port value check, in xen-unstable:19964. Can you ack/nack this please? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wed, 2010-01-20 at 16:07 +0000, Keir Fraser wrote:> On 20/01/2010 16:02, "Ky Srinivasan" <ksrinivasan@novell.com> wrote: > > > 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 <ksrinivasan@novell.com> > > 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 :-(> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 21/01/2010 20:38, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote:>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 1/21/2010 at 3:38 PM, in message< 1264106300.21707.57.camel@localhost.localdomain >, Ian Campbell < Ian.Campbell@citrix.com > wrote:> On Wed, 2010-01-20 at 16:07 +0000, Keir Fraser wrote: >> On 20/01/2010 16:02, "Ky Srinivasan" < ksrinivasan@novell.com > wrote: >> >> > 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 < ksrinivasan@novell.com > >> >> 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 :-( > >> Can you ack/nack this please? > > What vintage of older back-ends are we talking about?We shipped sles11 on xen-3.3.1 and this is where we encountered the problem - hosting a sles11 sp1 (based on xen-4.0.0) guest on a sles11 host.> > 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?Looking at your documentation for this new protocol, I recall seeing that if the magic value was not read, it was ok to silently return to be compatible.> > 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?We have had a mechanism for disabling the emulation when the PV drivers are loaded for some time now.> > 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.This breaks the compatibility with systems that have already been shipped in the sense we cannot run the guests with PV drivers. The proposed patch fixes the problem. Regards, K. Y> > 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._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2010-01-21 at 21:46 +0000, Keir Fraser wrote:> On 21/01/2010 20:38, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote: > > >> 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...> > 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.> Some people do want unplug though, so I think making it a non-default module > option is a good idea. > > -- Keir > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2010-01-21 at 23:04 +0000, Ky Srinivasan wrote:> > >>> On 1/21/2010 at 3:38 PM, in message > < 1264106300.21707.57.camel@localhost.localdomain >, Ian Campbell > < Ian.Campbell@citrix.com > wrote: > > On Wed, 2010-01-20 at 16:07 +0000, Keir Fraser wrote: > >> On 20/01/2010 16:02, "Ky Srinivasan" < ksrinivasan@novell.com > wrote: > >> > >> > 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 < ksrinivasan@novell.com > > >> > >> 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 :-( > > > >> Can you ack/nack this please? > > > > What vintage of older back-ends are we talking about?> We shipped sles11 on xen-3.3.1 and this is where we encountered the > problem - hosting a sles11 sp1 (based on xen-4.0.0) guest on a sles11 > host.OK, so not ancient.> > > > 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? > > Looking at your documentation for this new protocol, I recall seeing > that if the magic value was not read, it was ok to silently return to > be compatible.Which docs? i386-dm/README.hvm-pv-magic-ioport-disable in the qemu-xen tree says: 1) When the drivers first come up, they check whether the unplug logic is available by reading a two-byte magic number from IO port 0x10. These should be 0x49d2. If the magic number doesn''t match, the drivers don''t do anything. I take that to mean the drivers should not load if the magic value doesn''t match.> > 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? > We have had a mechanism for disabling the emulation when the PV > drivers are loaded for some time now.What is the mechanism? Why doesn''t your patch add support for that mechanism instead of just nobbling the current one?> > 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. > > This breaks the compatibility with systems that have already been > shipped in the sense we cannot run the guests with PV drivers. The > proposed patch fixes the problem.The guest still work though, right? Just with emulated drivers. If you add a module option override instead of just skipping the safety latch which the current mechanism implements then this patch would be OK and it would be a simple guest tweak to switch PV drivers back on if you have arranged out-of-band for the emulated devices to be unplugged. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/01/2010 07:31, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote:> 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?> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2010-01-22 at 08:03 +0000, Keir Fraser wrote:> On 22/01/2010 07:31, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote: > > > 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? > > > 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.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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/01/2010 08:43, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote:>>> 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. > > 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.> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/01/2010 08:43, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote:>> 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 Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, Jan 22, 2010 at 10:45:06AM +0000, Keir Fraser wrote:> On 22/01/2010 08:43, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote: > > >> 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. >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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, Jan 22, 2010 at 01:16:29PM +0200, Pasi Kärkkäinen wrote:> On Fri, Jan 22, 2010 at 10:45:06AM +0000, Keir Fraser wrote: > > On 22/01/2010 08:43, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote: > > > > >> 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. > > > > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 26/01/2010 10:12, "Pasi Kärkkäinen" <pasik@iki.fi> wrote:>>> 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. >> > > Btw against what kernels did you try to build?Vanilla 2.6.19 and vanilla 2.6.27. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel