Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Thu, 16 Jul 2015 19:22:21 +0200 Paolo Bonzini pbonz...@redhat.com wrote: On 16/07/2015 14:47, Michael S. Tsirkin wrote: I think for 2.4 it's a good idea to avoid enabling modern interface by default. Therefore, for 2.4 we can keep scsi enabled unless modern is requested by user. I agree. For 2.4, ccw isn't supporting revisions 0 anyway, so keeping scsi enabled for 2.4 sounds good to me as well.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Thu, Jul 16, 2015 at 02:37:12PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 21:51:32 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:39:18 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:11:57 +0300 Michael S. Tsirkin m...@redhat.com wrote: Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What's wrong with adding a disable-modern flag, like pci has? User can set that to get a legacy device. The whole idea behind the revision-stuff was that we don't need something like disable-modern. If the device is able to figure out on its own if it is to act as a modern or a legacy device, why require user intervention? It's about compatibility, e.g. being able to test legacy mode in transitional drivers in guests. Let's step back a bit. Why do we need legacy? To support old hosts and old guests. So what we need is a test matrix combining old/modern hosts with old/modern guests. I don't think forcing to an old version is something we should spend time on except during development. Consider also bugs, e.g. the fact that linux guests lack WCE in modern mode ATM. If we consider this a bug, shouldn't VERSION_1 be forced off unconditionally until specs and codebase are fixed? I don't see why. We'll never fix all bugs. It causes a performance regression but otherwise things work correctly. modern is off by default for now, this seems sufficient, if we never even ship it as an option we'll never find all bugs. What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it. Will have to think about it. But I think a flag to disable/enable modern is useful in any case, and it seems sufficient. I don't like the idea of disabling modern or legacy for ccw, where the differences between both are very minor. I also don't think requiring the user to specify a new flag on upgrade just to present the same features as before is a good idea: it is something that is easily missed and may lead to much headscratching. And doing this on a driver upgrade won't? As I said, if you believe this feature has value, argue that we shouldn't drop scsi off in virtio 1.0 then. I seem to have problems explaining myself :( I don't care about the feature, except for supporting old guests that should continue working as before even if the host updated. And without special configuration on the host, as this is too easily forgotten. What else is legacy good for, other than providing backwards compatibility? Maybe it's me having trouble explaining. There are two kind of blk devices: - those that allow scsi passthrough Such devices can't work properly through the modern interface. We could add a modern interface but device can not operate through it. - those that don't allow scsi passthrough Such devices can't work properly through the modern interface. We could add a modern interface and we'll get a transitional device. I think for 2.4 it's a good idea to avoid enabling modern interface by default. Therefore, for 2.4 we can keep scsi enabled unless modern is requested by user. I am also fine with just doing if (modern scsi) exit; -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, 15 Jul 2015 21:51:32 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:39:18 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:11:57 +0300 Michael S. Tsirkin m...@redhat.com wrote: Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What's wrong with adding a disable-modern flag, like pci has? User can set that to get a legacy device. The whole idea behind the revision-stuff was that we don't need something like disable-modern. If the device is able to figure out on its own if it is to act as a modern or a legacy device, why require user intervention? It's about compatibility, e.g. being able to test legacy mode in transitional drivers in guests. Let's step back a bit. Why do we need legacy? To support old hosts and old guests. So what we need is a test matrix combining old/modern hosts with old/modern guests. I don't think forcing to an old version is something we should spend time on except during development. Consider also bugs, e.g. the fact that linux guests lack WCE in modern mode ATM. If we consider this a bug, shouldn't VERSION_1 be forced off unconditionally until specs and codebase are fixed? What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it. Will have to think about it. But I think a flag to disable/enable modern is useful in any case, and it seems sufficient. I don't like the idea of disabling modern or legacy for ccw, where the differences between both are very minor. I also don't think requiring the user to specify a new flag on upgrade just to present the same features as before is a good idea: it is something that is easily missed and may lead to much headscratching. And doing this on a driver upgrade won't? As I said, if you believe this feature has value, argue that we shouldn't drop scsi off in virtio 1.0 then. I seem to have problems explaining myself :( I don't care about the feature, except for supporting old guests that should continue working as before even if the host updated. And without special configuration on the host, as this is too easily forgotten. What else is legacy good for, other than providing backwards compatibility?
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 16/07/2015 14:47, Michael S. Tsirkin wrote: I think for 2.4 it's a good idea to avoid enabling modern interface by default. Therefore, for 2.4 we can keep scsi enabled unless modern is requested by user. I agree. I am also fine with just doing if (modern scsi) exit; exit is evil. Paolo
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, 15 Jul 2015 13:59:00 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. I'm not opposing this :) If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. This seems to boil down to the different meaning of transitional for ccw and pci, see the other thread.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 13:59:00 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? Absolutely. For now one has to enable the modern interface explicitly. Around 2.5 we might switch that around, we'll need to think hard about compatibility at that point. In any case, we must definitely keep the old capability for old machine types. I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. I'm not opposing this :) If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. This seems to boil down to the different meaning of transitional for ccw and pci, see the other thread. Before the revision is negotiated, ccw won't know whether it's a legacy driver - is that the difference? Fine, but revision is negotiated way before features are probed so why does it make a practical difference? -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, 15 Jul 2015 15:01:01 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 13:59:00 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? Absolutely. For now one has to enable the modern interface explicitly. Around 2.5 we might switch that around, we'll need to think hard about compatibility at that point. In any case, we must definitely keep the old capability for old machine types. ccw only offers revision 0 (legacy) in 2.4. I plan to introduce revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 is the first versioned ccw machine). I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. I'm not opposing this :) If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. This seems to boil down to the different meaning of transitional for ccw and pci, see the other thread. Before the revision is negotiated, ccw won't know whether it's a legacy driver - is that the difference? I'd say it doesn't know whether the driver intends to use the modern interface. Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:39:18 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:11:57 +0300 Michael S. Tsirkin m...@redhat.com wrote: Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What's wrong with adding a disable-modern flag, like pci has? User can set that to get a legacy device. The whole idea behind the revision-stuff was that we don't need something like disable-modern. If the device is able to figure out on its own if it is to act as a modern or a legacy device, why require user intervention? It's about compatibility, e.g. being able to test legacy mode in transitional drivers in guests. Consider also bugs, e.g. the fact that linux guests lack WCE in modern mode ATM. What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it. Will have to think about it. But I think a flag to disable/enable modern is useful in any case, and it seems sufficient. I don't like the idea of disabling modern or legacy for ccw, where the differences between both are very minor. I also don't think requiring the user to specify a new flag on upgrade just to present the same features as before is a good idea: it is something that is easily missed and may lead to much headscratching. And doing this on a driver upgrade won't? As I said, if you believe this feature has value, argue that we shouldn't drop scsi off in virtio 1.0 then. -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, 15 Jul 2015 17:11:57 +0300 Michael S. Tsirkin m...@redhat.com wrote: Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, Jul 15, 2015 at 03:40:22PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 16:16:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 15:01:01 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 13:59:00 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? Absolutely. For now one has to enable the modern interface explicitly. Around 2.5 we might switch that around, we'll need to think hard about compatibility at that point. In any case, we must definitely keep the old capability for old machine types. ccw only offers revision 0 (legacy) in 2.4. I plan to introduce revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 is the first versioned ccw machine). I was talking about pci here actually. Sure, and these are my plans for ccw ;) I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. I'm not opposing this :) If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. This seems to boil down to the different meaning of transitional for ccw and pci, see the other thread. Before the revision is negotiated, ccw won't know whether it's a legacy driver - is that the difference? I'd say it doesn't know whether the driver intends to use the modern interface. That's also the case for pci. But does pci know the moment it first tries to get the device's features? And does pci assume modern as default for transitional devices? I don't think it does. Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 15:01:01 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 13:59:00 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? Absolutely. For now one has to enable the modern interface explicitly. Around 2.5 we might switch that around, we'll need to think hard about compatibility at that point. In any case, we must definitely keep the old capability for old machine types. ccw only offers revision 0 (legacy) in 2.4. I plan to introduce revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 is the first versioned ccw machine). I was talking about pci here actually. I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. I'm not opposing this :) If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. This seems to boil down to the different meaning of transitional for ccw and pci, see the other thread. Before the revision is negotiated, ccw won't know whether it's a legacy driver - is that the difference? I'd say it doesn't know whether the driver intends to use the modern interface. That's also the case for pci. Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, 15 Jul 2015 16:16:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 15:01:01 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 13:59:00 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision = 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? Absolutely. For now one has to enable the modern interface explicitly. Around 2.5 we might switch that around, we'll need to think hard about compatibility at that point. In any case, we must definitely keep the old capability for old machine types. ccw only offers revision 0 (legacy) in 2.4. I plan to introduce revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 is the first versioned ccw machine). I was talking about pci here actually. Sure, and these are my plans for ccw ;) I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. I'm not opposing this :) If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode. This seems to boil down to the different meaning of transitional for ccw and pci, see the other thread. Before the revision is negotiated, ccw won't know whether it's a legacy driver - is that the difference? I'd say it doesn't know whether the driver intends to use the modern interface. That's also the case for pci. But does pci know the moment it first tries to get the device's features? And does pci assume modern as default for transitional devices? Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, 15 Jul 2015 17:39:18 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:11:57 +0300 Michael S. Tsirkin m...@redhat.com wrote: Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What's wrong with adding a disable-modern flag, like pci has? User can set that to get a legacy device. The whole idea behind the revision-stuff was that we don't need something like disable-modern. If the device is able to figure out on its own if it is to act as a modern or a legacy device, why require user intervention? What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it. Will have to think about it. But I think a flag to disable/enable modern is useful in any case, and it seems sufficient. I don't like the idea of disabling modern or legacy for ccw, where the differences between both are very minor. I also don't think requiring the user to specify a new flag on upgrade just to present the same features as before is a good idea: it is something that is easily missed and may lead to much headscratching.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: On Wed, 15 Jul 2015 17:11:57 +0300 Michael S. Tsirkin m...@redhat.com wrote: Fine, but revision is negotiated way before features are probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set. Right. So simply do if (revision 1) return features 0x and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What's wrong with adding a disable-modern flag, like pci has? User can set that to get a legacy device. What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it. Will have to think about it. But I think a flag to disable/enable modern is useful in any case, and it seems sufficient. -- MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 18:35:53 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 15:36:00 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 15:22:52 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable
it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is
true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag
and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature
bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
We already handle this, do we not?
(...)
So guest that doesn't negotiate revision = 1 never gets to see
VIRTIO_F_VERSION_1.
Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?
No, I don't think there's a lot of value in offering scsi only to
old guests that don't negotiate revision = 1.
If user asked for virtio 1 support then that by proxy implies scsi
passthrough does not work, and it won't work for legacy
guests too.
This would imply that any transitional device cannot offer scsi,
doesn't it?
Yes, and that's because as written, transitional devices must set
ANY_LAYOUT, but that's incompatible with scsi.
Hm, I had a patch before that dynamically allowed different feature
sets for legacy or modern, not only a subset. Probably won't apply
anymore, but I'd like to able to do the following:
- driver reads features without negotiating a revision: driver is
legacy, offer legacy bits
- driver negotiates revision 0: dito
- driver negotiates revision = 1: driver is modern, offer modern bits
That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
first two cases, and a new qemu could still offer scsi to old guests.
Would it be worth pursuing that idea?
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 15:22:52 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
We already handle this, do we not?
(...)
So guest that doesn't negotiate revision = 1 never gets to see
VIRTIO_F_VERSION_1.
Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?
Maybe we should go further and additionally all bits = 32 if
VIRTIO_F_VERSION_1 is clear, but that can wait
and we have no bits like that in 2.4.
Spec says bits = 32 are only valid if we have VERSION_1, doesn't it?
Sounds sensible.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 05:00:51PM +0800, Jason Wang wrote:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Right. Auto would then mean !modern.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
---
hw/block/virtio-blk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..f30ad25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
*vdev, uint64_t features)
virtio_add_feature(features, VIRTIO_BLK_F_GEOMETRY);
virtio_add_feature(features, VIRTIO_BLK_F_TOPOLOGY);
virtio_add_feature(features, VIRTIO_BLK_F_BLK_SIZE);
-virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
+if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
+virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
if (s-conf.config_wce) {
virtio_add_feature(features, VIRTIO_BLK_F_CONFIG_WCE);
--
2.1.4
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 15:22:52 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
We already handle this, do we not?
(...)
So guest that doesn't negotiate revision = 1 never gets to see
VIRTIO_F_VERSION_1.
Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?
No, I don't think there's a lot of value in offering scsi only to
old guests that don't negotiate revision = 1.
If user asked for virtio 1 support then that by proxy implies scsi
passthrough does not work, and it won't work for legacy
guests too.
Maybe we should go further and additionally all bits = 32 if
VIRTIO_F_VERSION_1 is clear, but that can wait
and we have no bits like that in 2.4.
Spec says bits = 32 are only valid if we have VERSION_1, doesn't it?
Sounds sensible.
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
---
hw/block/virtio-blk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..f30ad25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
*vdev, uint64_t features)
virtio_add_feature(features, VIRTIO_BLK_F_GEOMETRY);
virtio_add_feature(features, VIRTIO_BLK_F_TOPOLOGY);
virtio_add_feature(features, VIRTIO_BLK_F_BLK_SIZE);
-virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
+if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
+virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
if (s-conf.config_wce) {
virtio_add_feature(features, VIRTIO_BLK_F_CONFIG_WCE);
--
2.1.4
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..f30ad25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
*vdev, uint64_t features)
virtio_add_feature(features, VIRTIO_BLK_F_GEOMETRY);
virtio_add_feature(features, VIRTIO_BLK_F_TOPOLOGY);
virtio_add_feature(features, VIRTIO_BLK_F_BLK_SIZE);
-virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
+if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
+virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
Coding style: Braces are needed here. (Same in the other patch you CCed
me on).
Kevin
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
We already handle this, do we not?
if (features.index == 0) {
features.features = (uint32_t)vdev-host_features;
} else if (features.index == 1) {
features.features = (uint32_t)(vdev-host_features 32);
/*
* Don't offer version 1 to the guest if it did not
* negotiate at least revision 1.
*/
if (dev-revision = 0) {
features.features = ~(1 (VIRTIO_F_VERSION_1 - 32));
}
} else {
/* Return zeroes if the guest supports more feature bits. */
features.features = 0;
}
So guest that doesn't negotiate revision = 1 never gets to see
VIRTIO_F_VERSION_1.
Maybe we should go further and additionally all bits = 32 if
VIRTIO_F_VERSION_1 is clear, but that can wait
and we have no bits like that in 2.4.
--
MST
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 15:36:00 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 15:22:52 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
We already handle this, do we not?
(...)
So guest that doesn't negotiate revision = 1 never gets to see
VIRTIO_F_VERSION_1.
Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?
No, I don't think there's a lot of value in offering scsi only to
old guests that don't negotiate revision = 1.
If user asked for virtio 1 support then that by proxy implies scsi
passthrough does not work, and it won't work for legacy
guests too.
This would imply that any transitional device cannot offer scsi,
doesn't it?
We have two layers interacting here: virtio-blk which may or may not
offer scsi support, and the transport layer which may or may not offer
VERSION_1 support. Failing scsi commands if VERSION_1 has been
negotiated makes sense to me; but I don't want to disable scsi config a
priori because the driver might negotiate VERSION_1. This would imply
that virtio-blk over virtio-ccw would never offer scsi once we enable
virtio-1 support, and it kind of defeats the purpose of a transitional
device for me.
(The other way round - fail negotiating revison 1 if the device was
configured with scsi support - makes more sense to me.)
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 13/07/2015 15:20, Cornelia Huck wrote: This would imply that any transitional device cannot offer scsi, doesn't it? We have two layers interacting here: virtio-blk which may or may not offer scsi support, and the transport layer which may or may not offer VERSION_1 support. Failing scsi commands if VERSION_1 has been negotiated makes sense to me; but I don't want to disable scsi config a priori because the driver might negotiate VERSION_1. This would imply that virtio-blk over virtio-ccw would never offer scsi once we enable virtio-1 support, and it kind of defeats the purpose of a transitional device for me. (The other way round - fail negotiating revison 1 if the device was configured with scsi support - makes more sense to me.) For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI altogether if !blk-conf.scsi. Would that fix the problem for you too? Paolo
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 16:34:30 +0200 Paolo Bonzini pbonz...@redhat.com wrote: On 13/07/2015 15:20, Cornelia Huck wrote: This would imply that any transitional device cannot offer scsi, doesn't it? We have two layers interacting here: virtio-blk which may or may not offer scsi support, and the transport layer which may or may not offer VERSION_1 support. Failing scsi commands if VERSION_1 has been negotiated makes sense to me; but I don't want to disable scsi config a priori because the driver might negotiate VERSION_1. This would imply that virtio-blk over virtio-ccw would never offer scsi once we enable virtio-1 support, and it kind of defeats the purpose of a transitional device for me. (The other way round - fail negotiating revison 1 if the device was configured with scsi support - makes more sense to me.) For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI altogether if !blk-conf.scsi. Would that fix the problem for you too? This is probably a sensible approach, and it can be contained in virtio-block, no?
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 13/07/2015 16:41, Cornelia Huck wrote: On Mon, 13 Jul 2015 16:34:30 +0200 Paolo Bonzini pbonz...@redhat.com wrote: On 13/07/2015 15:20, Cornelia Huck wrote: This would imply that any transitional device cannot offer scsi, doesn't it? We have two layers interacting here: virtio-blk which may or may not offer scsi support, and the transport layer which may or may not offer VERSION_1 support. Failing scsi commands if VERSION_1 has been negotiated makes sense to me; but I don't want to disable scsi config a priori because the driver might negotiate VERSION_1. This would imply that virtio-blk over virtio-ccw would never offer scsi once we enable virtio-1 support, and it kind of defeats the purpose of a transitional device for me. (The other way round - fail negotiating revison 1 if the device was configured with scsi support - makes more sense to me.) For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI altogether if !blk-conf.scsi. Would that fix the problem for you too? This is probably a sensible approach, and it can be contained in virtio-block, no? Yes, I think so. Paolo
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 15:36:00 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 15:22:52 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable
it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk-conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk-conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision = 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
We already handle this, do we not?
(...)
So guest that doesn't negotiate revision = 1 never gets to see
VIRTIO_F_VERSION_1.
Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?
No, I don't think there's a lot of value in offering scsi only to
old guests that don't negotiate revision = 1.
If user asked for virtio 1 support then that by proxy implies scsi
passthrough does not work, and it won't work for legacy
guests too.
This would imply that any transitional device cannot offer scsi,
doesn't it?
Yes, and that's because as written, transitional devices must set
ANY_LAYOUT, but that's incompatible with scsi.
We have two layers interacting here: virtio-blk which may or may not
offer scsi support, and the transport layer which may or may not offer
VERSION_1 support. Failing scsi commands if VERSION_1 has been
negotiated makes sense to me; but I don't want to disable scsi config a
priori because the driver might negotiate VERSION_1. This would imply
that virtio-blk over virtio-ccw would never offer scsi once we enable
virtio-1 support, and it kind of defeats the purpose of a transitional
device for me.
(The other way round - fail negotiating revison 1 if the device was
configured with scsi support - makes more sense to me.)
Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini pbonz...@redhat.com
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
---
hw/block/virtio-blk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..f30ad25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
*vdev, uint64_t features)
virtio_add_feature(features, VIRTIO_BLK_F_GEOMETRY);
virtio_add_feature(features, VIRTIO_BLK_F_TOPOLOGY);
virtio_add_feature(features, VIRTIO_BLK_F_BLK_SIZE);
-virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
+if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
+virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
if (s-conf.config_wce) {
virtio_add_feature(features, VIRTIO_BLK_F_CONFIG_WCE);
--
2.1.4
[Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-bl...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
---
hw/block/virtio-blk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..f30ad25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev,
uint64_t features)
virtio_add_feature(features, VIRTIO_BLK_F_GEOMETRY);
virtio_add_feature(features, VIRTIO_BLK_F_TOPOLOGY);
virtio_add_feature(features, VIRTIO_BLK_F_BLK_SIZE);
-virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
+if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
+virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
if (s-conf.config_wce) {
virtio_add_feature(features, VIRTIO_BLK_F_CONFIG_WCE);
--
2.1.4
