Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Wed, 24 Feb 2021 18:00:43 +0100 > Vitaly Kuznetsov wrote: > >> Igor Mammedov writes: >> >> > On Tue, 23 Feb 2021 19:08:42 +0100 >> > Vitaly Kuznetsov wrote: >> > >> >> Igor Mammedov writes: >> >> >> >> > On Tue, 23 Feb 2021 16:46:50 +0100 >> >> > Vitaly Kuznetsov wrote: >> >> > >> >> >> Igor Mammedov writes: >> >> >> >> >> >> > On Mon, 22 Feb 2021 11:20:34 +0100 >> >> >> > Vitaly Kuznetsov wrote: >> >> >> > >> >> >> >> Vitaly Kuznetsov writes: >> >> >> >> >> >> >> >> > Igor Mammedov writes: >> >> >> >> > >> >> >> >> >>> >> >> >> >> >>> We need to distinguish because that would be sane. >> >> >> >> >>> >> >> >> >> >>> Enlightened VMCS is an extension to VMX, it can't be used >> >> >> >> >>> without >> >> >> >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and >> >> >> >> >>> disabling it, >> >> >> >> >> ... >> >> >> >> >>> That bein said, if >> >> >> >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. >> >> >> >> >>> However, >> >> >> >> >>> there is a problem with explicit enablement: what should >> >> >> >> >>> >> >> >> >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? >> >> >> >> >>> Doesn't >> >> >> >> >>> sound sane to me. >> >> >> >> >> based on above I'd error out is user asks for unsupported option >> >> >> >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out >> >> >> >> >> >> >> >> >> > >> >> >> >> > That's what I keep telling you but you don't seem to listen. >> >> >> >> > 'Scratch >> >> >> >> > CPU' can't possibly help with this use-case because when you >> >> >> >> > parse >> >> >> >> > >> >> >> >> > 'hv-passthrough,hv-evmcs,vmx=off' you >> >> >> >> > >> >> >> >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by >> >> >> >> > the >> >> >> >> > host. >> >> >> >> > >> >> >> >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' >> >> >> >> > >> >> >> >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. >> >> >> >> > >> >> >> >> > We have to remember which options were aquired from the host and >> >> >> >> > which >> >> >> >> > were set explicitly by the user. >> >> >> >> >> >> >> >> Igor, >> >> >> >> >> >> >> >> could you please comment on the above? In case my line of thought is >> >> >> >> correct, and it is impossible to distinguish between e.g. >> >> >> >> >> >> >> >> 'hv-passthrough,hv-evmcs,-vmx' >> >> >> >> and >> >> >> >> 'hv-passthrough,-vmx' >> >> >> >> >> >> >> >> without a custom parser (written just exactly the way I did in this >> >> >> >> version, for example) regardless of when 'hv-passthrough' is >> >> >> >> expanded. E.g. we have the exact same problem with >> >> >> >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in >> >> >> >> discussing >> >> >> > >> >> >> > right, if we need to distinguish between explicit and implicit >> >> >> > hv-evmcs set by >> >> >> > hv-passthrough custom parser probably the way to go. >> >> >> > >> >> >> > However do we need actually need to do it? >> >> >> >> >> >> I think we really need that. See below ... >> >> >> >> >> >> > I'd treat 'hv-passthrough,-vmx' the same way as >> >> >> > 'hv-passthrough,hv-evmcs,-vmx' >> >> >> > and it applies not only hv-evmcs but other features hv-passthrough >> >> >> > might set >> >> >> > (i.e. if whatever was [un]set by hv-passthrough in combination with >> >> >> > other >> >> >> > features results in invalid config, QEMU shall error out instead of >> >> >> > magically >> >> >> > altering host provided hv-passthrough value). >> >> >> > >> >> >> > something like: >> >> >> > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set >> >> >> > should result in >> >> >> > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is >> >> >> > enabled," >> >> >> > " either enable 'vmx' or disable 'hv-evmcs' along >> >> >> > with disabling 'vmx'" >> >> >> > >> >> >> > making host's features set, *magically* mutable, depending on other >> >> >> > user provided features >> >> >> > is a bit confusing. One would never know what hv-passthrough >> >> >> > actually means, and if >> >> >> > enabling/disabling 'random' feature changes it. >> >> >> > >> >> >> > It's cleaner to do just what user asked (whether implicitly or >> >> >> > explicitly) and error out >> >> >> > in case it ends up in nonsense configuration. >> >> >> > >> >> >> >> >> >> I don't seem to agree this is a sane behavior, especially if you >> >> >> replace >> >> >> 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for >> >> >> Windows guests is common if you'd want to avoid nested configuration: >> >> >> even without any Hyper-V guests created, Windows itself is a Hyper-V >> >> >> partition. >> >> >> >> >> >> So a sane user will do: >> >> >> >> >> >> '-cpu host,hv-default,vmx=off' >> >> >> >> >> >> and on Intel he will get an error, and on AMD he won't. >> >>
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Wed, 24 Feb 2021 18:00:43 +0100 Vitaly Kuznetsov wrote: > Igor Mammedov writes: > > > On Tue, 23 Feb 2021 19:08:42 +0100 > > Vitaly Kuznetsov wrote: > > > >> Igor Mammedov writes: > >> > >> > On Tue, 23 Feb 2021 16:46:50 +0100 > >> > Vitaly Kuznetsov wrote: > >> > > >> >> Igor Mammedov writes: > >> >> > >> >> > On Mon, 22 Feb 2021 11:20:34 +0100 > >> >> > Vitaly Kuznetsov wrote: > >> >> > > >> >> >> Vitaly Kuznetsov writes: > >> >> >> > >> >> >> > Igor Mammedov writes: > >> >> >> > > >> >> >> >>> > >> >> >> >>> We need to distinguish because that would be sane. > >> >> >> >>> > >> >> >> >>> Enlightened VMCS is an extension to VMX, it can't be used without > >> >> >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and > >> >> >> >>> disabling it, > >> >> >> >> ... > >> >> >> >>> That bein said, if > >> >> >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. > >> >> >> >>> However, > >> >> >> >>> there is a problem with explicit enablement: what should > >> >> >> >>> > >> >> >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? > >> >> >> >>> Doesn't > >> >> >> >>> sound sane to me. > >> >> >> >> based on above I'd error out is user asks for unsupported option > >> >> >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out > >> >> >> >> > >> >> >> > > >> >> >> > That's what I keep telling you but you don't seem to listen. > >> >> >> > 'Scratch > >> >> >> > CPU' can't possibly help with this use-case because when you parse > >> >> >> > > >> >> >> > 'hv-passthrough,hv-evmcs,vmx=off' you > >> >> >> > > >> >> >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by > >> >> >> > the > >> >> >> > host. > >> >> >> > > >> >> >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' > >> >> >> > > >> >> >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. > >> >> >> > > >> >> >> > We have to remember which options were aquired from the host and > >> >> >> > which > >> >> >> > were set explicitly by the user. > >> >> >> > >> >> >> Igor, > >> >> >> > >> >> >> could you please comment on the above? In case my line of thought is > >> >> >> correct, and it is impossible to distinguish between e.g. > >> >> >> > >> >> >> 'hv-passthrough,hv-evmcs,-vmx' > >> >> >> and > >> >> >> 'hv-passthrough,-vmx' > >> >> >> > >> >> >> without a custom parser (written just exactly the way I did in this > >> >> >> version, for example) regardless of when 'hv-passthrough' is > >> >> >> expanded. E.g. we have the exact same problem with > >> >> >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing > >> >> >> > >> >> > > >> >> > right, if we need to distinguish between explicit and implicit > >> >> > hv-evmcs set by > >> >> > hv-passthrough custom parser probably the way to go. > >> >> > > >> >> > However do we need actually need to do it? > >> >> > >> >> I think we really need that. See below ... > >> >> > >> >> > I'd treat 'hv-passthrough,-vmx' the same way as > >> >> > 'hv-passthrough,hv-evmcs,-vmx' > >> >> > and it applies not only hv-evmcs but other features hv-passthrough > >> >> > might set > >> >> > (i.e. if whatever was [un]set by hv-passthrough in combination with > >> >> > other > >> >> > features results in invalid config, QEMU shall error out instead of > >> >> > magically > >> >> > altering host provided hv-passthrough value). > >> >> > > >> >> > something like: > >> >> > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set > >> >> > should result in > >> >> > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is > >> >> > enabled," > >> >> > " either enable 'vmx' or disable 'hv-evmcs' along > >> >> > with disabling 'vmx'" > >> >> > > >> >> > making host's features set, *magically* mutable, depending on other > >> >> > user provided features > >> >> > is a bit confusing. One would never know what hv-passthrough actually > >> >> > means, and if > >> >> > enabling/disabling 'random' feature changes it. > >> >> > > >> >> > It's cleaner to do just what user asked (whether implicitly or > >> >> > explicitly) and error out > >> >> > in case it ends up in nonsense configuration. > >> >> > > >> >> > >> >> I don't seem to agree this is a sane behavior, especially if you replace > >> >> 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for > >> >> Windows guests is common if you'd want to avoid nested configuration: > >> >> even without any Hyper-V guests created, Windows itself is a Hyper-V > >> >> partition. > >> >> > >> >> So a sane user will do: > >> >> > >> >> '-cpu host,hv-default,vmx=off' > >> >> > >> >> and on Intel he will get an error, and on AMD he won't. > >> >> > >> >> So what you're suggesting actually defeats the whole purpose of > >> >> 'hv-default' as upper-layer tools (think libvirt) will need to know > >> >> that > >> > I'd assume it
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Tue, 23 Feb 2021 19:08:42 +0100 > Vitaly Kuznetsov wrote: > >> Igor Mammedov writes: >> >> > On Tue, 23 Feb 2021 16:46:50 +0100 >> > Vitaly Kuznetsov wrote: >> > >> >> Igor Mammedov writes: >> >> >> >> > On Mon, 22 Feb 2021 11:20:34 +0100 >> >> > Vitaly Kuznetsov wrote: >> >> > >> >> >> Vitaly Kuznetsov writes: >> >> >> >> >> >> > Igor Mammedov writes: >> >> >> > >> >> >> >>> >> >> >> >>> We need to distinguish because that would be sane. >> >> >> >>> >> >> >> >>> Enlightened VMCS is an extension to VMX, it can't be used without >> >> >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling >> >> >> >>> it, >> >> >> >> ... >> >> >> >>> That bein said, if >> >> >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. >> >> >> >>> However, >> >> >> >>> there is a problem with explicit enablement: what should >> >> >> >>> >> >> >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? >> >> >> >>> Doesn't >> >> >> >>> sound sane to me. >> >> >> >> based on above I'd error out is user asks for unsupported option >> >> >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out >> >> >> > >> >> >> > That's what I keep telling you but you don't seem to listen. 'Scratch >> >> >> > CPU' can't possibly help with this use-case because when you parse >> >> >> > >> >> >> > 'hv-passthrough,hv-evmcs,vmx=off' you >> >> >> > >> >> >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the >> >> >> > host. >> >> >> > >> >> >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' >> >> >> > >> >> >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. >> >> >> > >> >> >> > We have to remember which options were aquired from the host and >> >> >> > which >> >> >> > were set explicitly by the user. >> >> >> >> >> >> Igor, >> >> >> >> >> >> could you please comment on the above? In case my line of thought is >> >> >> correct, and it is impossible to distinguish between e.g. >> >> >> >> >> >> 'hv-passthrough,hv-evmcs,-vmx' >> >> >> and >> >> >> 'hv-passthrough,-vmx' >> >> >> >> >> >> without a custom parser (written just exactly the way I did in this >> >> >> version, for example) regardless of when 'hv-passthrough' is >> >> >> expanded. E.g. we have the exact same problem with >> >> >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing >> >> >> >> >> > >> >> > right, if we need to distinguish between explicit and implicit hv-evmcs >> >> > set by >> >> > hv-passthrough custom parser probably the way to go. >> >> > >> >> > However do we need actually need to do it? >> >> >> >> I think we really need that. See below ... >> >> >> >> > I'd treat 'hv-passthrough,-vmx' the same way as >> >> > 'hv-passthrough,hv-evmcs,-vmx' >> >> > and it applies not only hv-evmcs but other features hv-passthrough >> >> > might set >> >> > (i.e. if whatever was [un]set by hv-passthrough in combination with >> >> > other >> >> > features results in invalid config, QEMU shall error out instead of >> >> > magically >> >> > altering host provided hv-passthrough value). >> >> > >> >> > something like: >> >> > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set >> >> > should result in >> >> > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is >> >> > enabled," >> >> > " either enable 'vmx' or disable 'hv-evmcs' along with >> >> > disabling 'vmx'" >> >> > >> >> > making host's features set, *magically* mutable, depending on other >> >> > user provided features >> >> > is a bit confusing. One would never know what hv-passthrough actually >> >> > means, and if >> >> > enabling/disabling 'random' feature changes it. >> >> > >> >> > It's cleaner to do just what user asked (whether implicitly or >> >> > explicitly) and error out >> >> > in case it ends up in nonsense configuration. >> >> > >> >> >> >> I don't seem to agree this is a sane behavior, especially if you replace >> >> 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for >> >> Windows guests is common if you'd want to avoid nested configuration: >> >> even without any Hyper-V guests created, Windows itself is a Hyper-V >> >> partition. >> >> >> >> So a sane user will do: >> >> >> >> '-cpu host,hv-default,vmx=off' >> >> >> >> and on Intel he will get an error, and on AMD he won't. >> >> >> >> So what you're suggesting actually defeats the whole purpose of >> >> 'hv-default' as upper-layer tools (think libvirt) will need to know that >> > I'd assume it would be hard for libvirt to use 'hv-default' from migration >> > point of view. It's semi opaque (one can find out what features it sets >> > indirectly inspecting individual hv_foo features, and mgmt will need to >> > know about them). If it will mutate when other features [un]set, upper >> > layers might need to enumerate all these permutations to know which hosts >> > are compati
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Tue, 23 Feb 2021 19:08:42 +0100 Vitaly Kuznetsov wrote: > Igor Mammedov writes: > > > On Tue, 23 Feb 2021 16:46:50 +0100 > > Vitaly Kuznetsov wrote: > > > >> Igor Mammedov writes: > >> > >> > On Mon, 22 Feb 2021 11:20:34 +0100 > >> > Vitaly Kuznetsov wrote: > >> > > >> >> Vitaly Kuznetsov writes: > >> >> > >> >> > Igor Mammedov writes: > >> >> > > >> >> >>> > >> >> >>> We need to distinguish because that would be sane. > >> >> >>> > >> >> >>> Enlightened VMCS is an extension to VMX, it can't be used without > >> >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling > >> >> >>> it, > >> >> >> ... > >> >> >>> That bein said, if > >> >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. > >> >> >>> However, > >> >> >>> there is a problem with explicit enablement: what should > >> >> >>> > >> >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? > >> >> >>> Doesn't > >> >> >>> sound sane to me. > >> >> >> based on above I'd error out is user asks for unsupported option > >> >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out > >> >> > > >> >> > That's what I keep telling you but you don't seem to listen. 'Scratch > >> >> > CPU' can't possibly help with this use-case because when you parse > >> >> > > >> >> > 'hv-passthrough,hv-evmcs,vmx=off' you > >> >> > > >> >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the > >> >> > host. > >> >> > > >> >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' > >> >> > > >> >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. > >> >> > > >> >> > We have to remember which options were aquired from the host and which > >> >> > were set explicitly by the user. > >> >> > >> >> Igor, > >> >> > >> >> could you please comment on the above? In case my line of thought is > >> >> correct, and it is impossible to distinguish between e.g. > >> >> > >> >> 'hv-passthrough,hv-evmcs,-vmx' > >> >> and > >> >> 'hv-passthrough,-vmx' > >> >> > >> >> without a custom parser (written just exactly the way I did in this > >> >> version, for example) regardless of when 'hv-passthrough' is > >> >> expanded. E.g. we have the exact same problem with > >> >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing > >> > > >> > right, if we need to distinguish between explicit and implicit hv-evmcs > >> > set by > >> > hv-passthrough custom parser probably the way to go. > >> > > >> > However do we need actually need to do it? > >> > >> I think we really need that. See below ... > >> > >> > I'd treat 'hv-passthrough,-vmx' the same way as > >> > 'hv-passthrough,hv-evmcs,-vmx' > >> > and it applies not only hv-evmcs but other features hv-passthrough might > >> > set > >> > (i.e. if whatever was [un]set by hv-passthrough in combination with other > >> > features results in invalid config, QEMU shall error out instead of > >> > magically > >> > altering host provided hv-passthrough value). > >> > > >> > something like: > >> > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set > >> > should result in > >> > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is > >> > enabled," > >> > " either enable 'vmx' or disable 'hv-evmcs' along with > >> > disabling 'vmx'" > >> > > >> > making host's features set, *magically* mutable, depending on other user > >> > provided features > >> > is a bit confusing. One would never know what hv-passthrough actually > >> > means, and if > >> > enabling/disabling 'random' feature changes it. > >> > > >> > It's cleaner to do just what user asked (whether implicitly or > >> > explicitly) and error out > >> > in case it ends up in nonsense configuration. > >> > > >> > >> I don't seem to agree this is a sane behavior, especially if you replace > >> 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for > >> Windows guests is common if you'd want to avoid nested configuration: > >> even without any Hyper-V guests created, Windows itself is a Hyper-V > >> partition. > >> > >> So a sane user will do: > >> > >> '-cpu host,hv-default,vmx=off' > >> > >> and on Intel he will get an error, and on AMD he won't. > >> > >> So what you're suggesting actually defeats the whole purpose of > >> 'hv-default' as upper-layer tools (think libvirt) will need to know that > > I'd assume it would be hard for libvirt to use 'hv-default' from migration > > point of view. It's semi opaque (one can find out what features it sets > > indirectly inspecting individual hv_foo features, and mgmt will need to > > know about them). If it will mutate when other features [un]set, upper > > layers might need to enumerate all these permutations to know which hosts > > are compatible or compare host feature sets every time before attempting > > migration. > > That's exactly the opposite of what's the goal here which is: make it > possible for upper laye
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Tue, 23 Feb 2021 16:46:50 +0100 > Vitaly Kuznetsov wrote: > >> Igor Mammedov writes: >> >> > On Mon, 22 Feb 2021 11:20:34 +0100 >> > Vitaly Kuznetsov wrote: >> > >> >> Vitaly Kuznetsov writes: >> >> >> >> > Igor Mammedov writes: >> >> > >> >> >>> >> >> >>> We need to distinguish because that would be sane. >> >> >>> >> >> >>> Enlightened VMCS is an extension to VMX, it can't be used without >> >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling >> >> >>> it, >> >> >> ... >> >> >>> That bein said, if >> >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. However, >> >> >>> there is a problem with explicit enablement: what should >> >> >>> >> >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't >> >> >>> sound sane to me. >> >> >> based on above I'd error out is user asks for unsupported option >> >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out >> >> > >> >> > That's what I keep telling you but you don't seem to listen. 'Scratch >> >> > CPU' can't possibly help with this use-case because when you parse >> >> > >> >> > 'hv-passthrough,hv-evmcs,vmx=off' you >> >> > >> >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the >> >> > host. >> >> > >> >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' >> >> > >> >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. >> >> > >> >> > We have to remember which options were aquired from the host and which >> >> > were set explicitly by the user. >> >> >> >> Igor, >> >> >> >> could you please comment on the above? In case my line of thought is >> >> correct, and it is impossible to distinguish between e.g. >> >> >> >> 'hv-passthrough,hv-evmcs,-vmx' >> >> and >> >> 'hv-passthrough,-vmx' >> >> >> >> without a custom parser (written just exactly the way I did in this >> >> version, for example) regardless of when 'hv-passthrough' is >> >> expanded. E.g. we have the exact same problem with >> >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing >> > >> > right, if we need to distinguish between explicit and implicit hv-evmcs >> > set by >> > hv-passthrough custom parser probably the way to go. >> > >> > However do we need actually need to do it? >> >> I think we really need that. See below ... >> >> > I'd treat 'hv-passthrough,-vmx' the same way as >> > 'hv-passthrough,hv-evmcs,-vmx' >> > and it applies not only hv-evmcs but other features hv-passthrough might >> > set >> > (i.e. if whatever was [un]set by hv-passthrough in combination with other >> > features results in invalid config, QEMU shall error out instead of >> > magically >> > altering host provided hv-passthrough value). >> > >> > something like: >> > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set >> > should result in >> > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is >> > enabled," >> > " either enable 'vmx' or disable 'hv-evmcs' along with >> > disabling 'vmx'" >> > >> > making host's features set, *magically* mutable, depending on other user >> > provided features >> > is a bit confusing. One would never know what hv-passthrough actually >> > means, and if >> > enabling/disabling 'random' feature changes it. >> > >> > It's cleaner to do just what user asked (whether implicitly or explicitly) >> > and error out >> > in case it ends up in nonsense configuration. >> > >> >> I don't seem to agree this is a sane behavior, especially if you replace >> 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for >> Windows guests is common if you'd want to avoid nested configuration: >> even without any Hyper-V guests created, Windows itself is a Hyper-V >> partition. >> >> So a sane user will do: >> >> '-cpu host,hv-default,vmx=off' >> >> and on Intel he will get an error, and on AMD he won't. >> >> So what you're suggesting actually defeats the whole purpose of >> 'hv-default' as upper-layer tools (think libvirt) will need to know that > I'd assume it would be hard for libvirt to use 'hv-default' from migration > point of view. It's semi opaque (one can find out what features it sets > indirectly inspecting individual hv_foo features, and mgmt will need to > know about them). If it will mutate when other features [un]set, upper > layers might need to enumerate all these permutations to know which hosts > are compatible or compare host feature sets every time before attempting > migration. > That's exactly the opposite of what's the goal here which is: make it possible for upper layers to not know anything about Hyper-V enlightenments besides 'hv-default'. Migration should work just fine, if the rest of guest configuration matches -- then 'hv-default' will create the exact same things (e.g. if 'vmx' was disabled on the source it has to be enabled on the destination, it can't be different) >> Intel configurations for Windows gue
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Tue, 23 Feb 2021 16:46:50 +0100 Vitaly Kuznetsov wrote: > Igor Mammedov writes: > > > On Mon, 22 Feb 2021 11:20:34 +0100 > > Vitaly Kuznetsov wrote: > > > >> Vitaly Kuznetsov writes: > >> > >> > Igor Mammedov writes: > >> > > >> >>> > >> >>> We need to distinguish because that would be sane. > >> >>> > >> >>> Enlightened VMCS is an extension to VMX, it can't be used without > >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling it, > >> >>> > >> >> ... > >> >>> That bein said, if > >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. However, > >> >>> there is a problem with explicit enablement: what should > >> >>> > >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't > >> >>> sound sane to me. > >> >> based on above I'd error out is user asks for unsupported option > >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out > >> > > >> > That's what I keep telling you but you don't seem to listen. 'Scratch > >> > CPU' can't possibly help with this use-case because when you parse > >> > > >> > 'hv-passthrough,hv-evmcs,vmx=off' you > >> > > >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the > >> > host. > >> > > >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' > >> > > >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. > >> > > >> > We have to remember which options were aquired from the host and which > >> > were set explicitly by the user. > >> > >> Igor, > >> > >> could you please comment on the above? In case my line of thought is > >> correct, and it is impossible to distinguish between e.g. > >> > >> 'hv-passthrough,hv-evmcs,-vmx' > >> and > >> 'hv-passthrough,-vmx' > >> > >> without a custom parser (written just exactly the way I did in this > >> version, for example) regardless of when 'hv-passthrough' is > >> expanded. E.g. we have the exact same problem with > >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing > > > > right, if we need to distinguish between explicit and implicit hv-evmcs set > > by > > hv-passthrough custom parser probably the way to go. > > > > However do we need actually need to do it? > > I think we really need that. See below ... > > > I'd treat 'hv-passthrough,-vmx' the same way as > > 'hv-passthrough,hv-evmcs,-vmx' > > and it applies not only hv-evmcs but other features hv-passthrough might set > > (i.e. if whatever was [un]set by hv-passthrough in combination with other > > features results in invalid config, QEMU shall error out instead of > > magically > > altering host provided hv-passthrough value). > > > > something like: > > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set > > should result in > > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is > > enabled," > > " either enable 'vmx' or disable 'hv-evmcs' along with > > disabling 'vmx'" > > > > making host's features set, *magically* mutable, depending on other user > > provided features > > is a bit confusing. One would never know what hv-passthrough actually > > means, and if > > enabling/disabling 'random' feature changes it. > > > > It's cleaner to do just what user asked (whether implicitly or explicitly) > > and error out > > in case it ends up in nonsense configuration. > > > > I don't seem to agree this is a sane behavior, especially if you replace > 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for > Windows guests is common if you'd want to avoid nested configuration: > even without any Hyper-V guests created, Windows itself is a Hyper-V > partition. > > So a sane user will do: > > '-cpu host,hv-default,vmx=off' > > and on Intel he will get an error, and on AMD he won't. > > So what you're suggesting actually defeats the whole purpose of > 'hv-default' as upper-layer tools (think libvirt) will need to know that I'd assume it would be hard for libvirt to use 'hv-default' from migration point of view. It's semi opaque (one can find out what features it sets indirectly inspecting individual hv_foo features, and mgmt will need to know about them). If it will mutate when other features [un]set, upper layers might need to enumerate all these permutations to know which hosts are compatible or compare host feature sets every time before attempting migration. > Intel configurations for Windows guests are somewhat different. They'll > need to know what 'hv-evmcs' is. We're back to where we've started. we were talking about hv-passthrough, and if host advertises hv-evmcs QEMU should complain if user disabled features it depends on ( not silently fixing up configuration error). But the same applies to hv-default. > If we are to follow this approach let's just throw away 'hv-evmcs' from > 'hv-default' set, it's going to be much cleaner. But again, I don't > really believe it's the right way to go. if desired behavior, on Intel host for above config,
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Mon, 22 Feb 2021 11:20:34 +0100 > Vitaly Kuznetsov wrote: > >> Vitaly Kuznetsov writes: >> >> > Igor Mammedov writes: >> > >> >>> >> >>> We need to distinguish because that would be sane. >> >>> >> >>> Enlightened VMCS is an extension to VMX, it can't be used without >> >>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling it, >> >> ... >> >>> That bein said, if >> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. However, >> >>> there is a problem with explicit enablement: what should >> >>> >> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't >> >>> sound sane to me. >> >> based on above I'd error out is user asks for unsupported option >> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out >> > >> > That's what I keep telling you but you don't seem to listen. 'Scratch >> > CPU' can't possibly help with this use-case because when you parse >> > >> > 'hv-passthrough,hv-evmcs,vmx=off' you >> > >> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the >> > host. >> > >> > 2) 'hv-evmcs' -> keep EVMCS bit '1' >> > >> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from. >> > >> > We have to remember which options were aquired from the host and which >> > were set explicitly by the user. >> >> Igor, >> >> could you please comment on the above? In case my line of thought is >> correct, and it is impossible to distinguish between e.g. >> >> 'hv-passthrough,hv-evmcs,-vmx' >> and >> 'hv-passthrough,-vmx' >> >> without a custom parser (written just exactly the way I did in this >> version, for example) regardless of when 'hv-passthrough' is >> expanded. E.g. we have the exact same problem with >> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing > > right, if we need to distinguish between explicit and implicit hv-evmcs set by > hv-passthrough custom parser probably the way to go. > > However do we need actually need to do it? I think we really need that. See below ... > I'd treat 'hv-passthrough,-vmx' the same way as 'hv-passthrough,hv-evmcs,-vmx' > and it applies not only hv-evmcs but other features hv-passthrough might set > (i.e. if whatever was [un]set by hv-passthrough in combination with other > features results in invalid config, QEMU shall error out instead of magically > altering host provided hv-passthrough value). > > something like: > 'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set > should result in > error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is enabled," > " either enable 'vmx' or disable 'hv-evmcs' along with > disabling 'vmx'" > > making host's features set, *magically* mutable, depending on other user > provided features > is a bit confusing. One would never know what hv-passthrough actually means, > and if > enabling/disabling 'random' feature changes it. > > It's cleaner to do just what user asked (whether implicitly or explicitly) > and error out > in case it ends up in nonsense configuration. > I don't seem to agree this is a sane behavior, especially if you replace 'hv-passthrough' with 'hv-default' above. Removing 'vmx' from CPU for Windows guests is common if you'd want to avoid nested configuration: even without any Hyper-V guests created, Windows itself is a Hyper-V partition. So a sane user will do: '-cpu host,hv-default,vmx=off' and on Intel he will get an error, and on AMD he won't. So what you're suggesting actually defeats the whole purpose of 'hv-default' as upper-layer tools (think libvirt) will need to know that Intel configurations for Windows guests are somewhat different. They'll need to know what 'hv-evmcs' is. We're back to where we've started. If we are to follow this approach let's just throw away 'hv-evmcs' from 'hv-default' set, it's going to be much cleaner. But again, I don't really believe it's the right way to go. -- Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Mon, 22 Feb 2021 11:20:34 +0100
Vitaly Kuznetsov wrote:
> Vitaly Kuznetsov writes:
>
> > Igor Mammedov writes:
> >
> >>>
> >>> We need to distinguish because that would be sane.
> >>>
> >>> Enlightened VMCS is an extension to VMX, it can't be used without
> >>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling it,
> >> ...
> >>> That bein said, if
> >>> guest CPU lacks VMX it is counter-productive to expose EVMCS. However,
> >>> there is a problem with explicit enablement: what should
> >>>
> >>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't
> >>> sound sane to me.
> >> based on above I'd error out is user asks for unsupported option
> >> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out
> >
> > That's what I keep telling you but you don't seem to listen. 'Scratch
> > CPU' can't possibly help with this use-case because when you parse
> >
> > 'hv-passthrough,hv-evmcs,vmx=off' you
> >
> > 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the
> > host.
> >
> > 2) 'hv-evmcs' -> keep EVMCS bit '1'
> >
> > 3) 'vmx=off' -> you have no idea where EVMCS bit came from.
> >
> > We have to remember which options were aquired from the host and which
> > were set explicitly by the user.
>
> Igor,
>
> could you please comment on the above? In case my line of thought is
> correct, and it is impossible to distinguish between e.g.
>
> 'hv-passthrough,hv-evmcs,-vmx'
> and
> 'hv-passthrough,-vmx'
>
> without a custom parser (written just exactly the way I did in this
> version, for example) regardless of when 'hv-passthrough' is
> expanded. E.g. we have the exact same problem with
> 'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing
right, if we need to distinguish between explicit and implicit hv-evmcs set by
hv-passthrough custom parser probably the way to go.
However do we need actually need to do it?
I'd treat 'hv-passthrough,-vmx' the same way as 'hv-passthrough,hv-evmcs,-vmx'
and it applies not only hv-evmcs but other features hv-passthrough might set
(i.e. if whatever was [un]set by hv-passthrough in combination with other
features results in invalid config, QEMU shall error out instead of magically
altering host provided hv-passthrough value).
something like:
'hv-passthrough,-vmx' when hv-passthrough makes hv-evmcs bit set
should result in
error_setg(errp,"'vmx' feature can't be disabled when hv-evmcs is enabled,"
" either enable 'vmx' or disable 'hv-evmcs' along with
disabling 'vmx'"
making host's features set, *magically* mutable, depending on other user
provided features
is a bit confusing. One would never know what hv-passthrough actually means,
and if
enabling/disabling 'random' feature changes it.
It's cleaner to do just what user asked (whether implicitly or explicitly) and
error out
in case it ends up in nonsense configuration.
> 'scratch CPUs' idea at this point because it is not going to change
> anything at all ('hv_features_on' will stay, custom parsers will stay).g
>
> Am I missing something?
>
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Vitaly Kuznetsov writes:
> Igor Mammedov writes:
>
>>>
>>> We need to distinguish because that would be sane.
>>>
>>> Enlightened VMCS is an extension to VMX, it can't be used without
>>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling it,
>> ...
>>> That bein said, if
>>> guest CPU lacks VMX it is counter-productive to expose EVMCS. However,
>>> there is a problem with explicit enablement: what should
>>>
>>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't
>>> sound sane to me.
>> based on above I'd error out is user asks for unsupported option
>> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out
>
> That's what I keep telling you but you don't seem to listen. 'Scratch
> CPU' can't possibly help with this use-case because when you parse
>
> 'hv-passthrough,hv-evmcs,vmx=off' you
>
> 1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the
> host.
>
> 2) 'hv-evmcs' -> keep EVMCS bit '1'
>
> 3) 'vmx=off' -> you have no idea where EVMCS bit came from.
>
> We have to remember which options were aquired from the host and which
> were set explicitly by the user.
Igor,
could you please comment on the above? In case my line of thought is
correct, and it is impossible to distinguish between e.g.
'hv-passthrough,hv-evmcs,-vmx'
and
'hv-passthrough,-vmx'
without a custom parser (written just exactly the way I did in this
version, for example) regardless of when 'hv-passthrough' is
expanded. E.g. we have the exact same problem with
'hv-default,hv-evmcs,-vmx'. I that case I see no point in discussing
'scratch CPUs' idea at this point because it is not going to change
anything at all ('hv_features_on' will stay, custom parsers will stay).
Am I missing something?
--
Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Mon, 15 Feb 2021 09:56:19 +0100 > Vitaly Kuznetsov wrote: > >> Igor Mammedov writes: >> >> > On Fri, 12 Feb 2021 16:26:03 +0100 >> > Vitaly Kuznetsov wrote: >> > >> >> Vitaly Kuznetsov writes: >> >> >> >> > Igor Mammedov writes: >> >> > >> >> >> >> >> >> Please try reusing scratch CPU approach, see >> >> >> kvm_arm_get_host_cpu_features() >> >> >> for an example. You will very likely end up with simpler series, >> >> >> compared to reinventing wheel. >> >> > >> >> > Even if I do that (and I serioulsy doubt it's going to be easier than >> >> > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 >> >> > lines long) this is not going to give us what we need to distinguish >> >> > between >> >> > >> >> > 'hv-passthrough,hv-evmcs' >> >> > >> >> > and >> >> > >> >> > 'hv-passthrough' >> >> > >> >> > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we >> >> > don't want to enable it unless it was requested explicitly (former but >> >> > not the later). >> >> >> >> ... and if for whatever reason we decide that this is also bad/not >> >> needed, I can just drop patches 16-18 from the series (leaving >> >> 'hv-passthrough,hv-feature=off' problem to better times). >> > that's also an option, >> > we would need to make sure that hv-passthrough is mutually exclusive >> > with ''all'' other hv- properties to avoid above combination being >> > ever (mis)used. >> >> That's an option to finally get these patches merged, not a good option >> for end users. >> >> 'hv-passthrough,hv-feature' works today and it's useful. Should we drop >> it? > well, > try suggested idea about using scratch CPU and it might get merged sooner. > (it's not like I'm suggesting you to rewrite half of QEMU, just some of > patches, which most likely would simplify series from my point of view > and would be easier to maintain) > I don't see anything in the series which will go away if I implement this idea but as I hate it deerly I'm likely not going to. -- Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes:
>>
>> We need to distinguish because that would be sane.
>>
>> Enlightened VMCS is an extension to VMX, it can't be used without
>> it. Genuine Hyper-V doesn't have a knob for enabling and disabling it,
> ...
>> That bein said, if
>> guest CPU lacks VMX it is counter-productive to expose EVMCS. However,
>> there is a problem with explicit enablement: what should
>>
>> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't
>> sound sane to me.
> based on above I'd error out is user asks for unsupported option
> i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out
That's what I keep telling you but you don't seem to listen. 'Scratch
CPU' can't possibly help with this use-case because when you parse
'hv-passthrough,hv-evmcs,vmx=off' you
1) "hv-passthrough" -> set EVMCS bit to '1' as it is supported by the
host.
2) 'hv-evmcs' -> keep EVMCS bit '1'
3) 'vmx=off' -> you have no idea where EVMCS bit came from.
We have to remember which options were aquired from the host and which
were set explicitly by the user. Ok, you can replace
'hyperv_features_on' with 'evmcs_was_explicitly_requested' but how is it
better?
>
> if later on we find usecase for VMX=off + hv-evmcs=on,
> we will be able to drop error without affecting existing users,
> but not other way around.
>
>> >> Moreover, instead of just adding two 'u64's we're now doing an ioctl
>> >> which can fail, be subject to limits,... Creating and destroying a CPU
>> >> is also slow. Sorry, I hardly see how this is better, maybe just from
>> >> 'code purity' point of view.
>> > readable and easy to maintain code is not a thing to neglect.
>>
>> Of couse, but 'scratch CPU' idea is not a good design decision, it is an
>> ugly hack we should get rid of in ARM land, not try bringing it to other
>> architectures. Generally, KVM should allow to query all its capabilities
>> without the need to create a vCPU or, if not possible, we should create
>> 'real' QEMU VCPUs and use one/all of the to query capabilities, avoiding
>> 'scratch' because:
>> - Creating and destroying a vCPU makes VM startup slower, much
>> slower. E.g. for a single-CPU VM you're doubling the time required to
>> create vCPUs!
>> - vCPUs in KVM are quite memory consuming. Just 'struct kvm_vcpu_arch'
>> was something like 12kb last time I looked at it.
>>
>> I have no clue why scratch vCPUs were implemented on ARM, however, I'd
>> very much want us to avoid doing the same on x86. We do have use-cases
>> where startup time and consumed memory is important. There is a point in
>> limiting ioctls for security reasons (e.g. if I'm creating a single vCPU
>> VM I may want to limit userspace process to one and only one
>> KVM_CREATE_VCPU call).
> it should be possible to reuse scratch VCPU (kvm file descriptor) as
> the first CPU of VM, if there is a will/need, without creating unnecessary
> overhead.
> I don't like scratch CPU either but from my pov it's a lesser evil to
> spawning custom parser every time someone fills like it.
I respectfully disagree.
>
>
>> Now to the code you complain about. The 'hard to read and maintain' code
>> is literaly this:
>>
>> +static void x86_hv_feature_set(Object *obj, bool value, int feature)
>> +{
>> +X86CPU *cpu = X86_CPU(obj);
>> +
>> +if (value) {
>> +cpu->hyperv_features |= BIT(feature);
>> +cpu->hyperv_features_on |= BIT(feature);
>> +cpu->hyperv_features_off &= ~BIT(feature);
>> +} else {
>> +cpu->hyperv_features &= ~BIT(feature);
>> +cpu->hyperv_features_on &= ~BIT(feature);
>> +cpu->hyperv_features_off |= BIT(feature);
>> +}
>> +}
> It's not just that code but the rest that uses above variables to
> get final hyperv_features feature set. There is a lot of invariants
> that are hidden in hv specific code that you put in hyperv kvm
> specific part.
Could you give an example please?
>
> btw why can't we get supported hyperv_features in passthrough mode
> during time we initialize KVM (without a vCPU)?
I think I already explained that: KVM_GET_SUPPORTED_HV_CPUID works on
KVM fd from 5.11, it requires a vCPU prior to that.
>
>> I can add as many comments here as needed, however, I don't see what
>> requires additional explanaition. We just want to know two things:
>> - What's the 'effective' setting of the control
>> - Was it explicitly enabled or disabled on the command line.
>>
>> Custom parsers are not new in QEMU and they're not going anywhere I
>> believe. There are options with simple enablent and there are some with
>> additional considerations. Trying to make CPU objects somewhat 'special'
>> by forcing all options to be of type-1 (and thus crippling user
>> experience) is not the way to go IMHO. I'd very much like us to go in
>> another direction, make our option parser better so my very simple
>> use-case is covered 'out-of-the-box'.
> there is a lot of effort spent on getting rid of custom parsers that
> QEMU accumulated ove
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Mon, 15 Feb 2021 16:55:02 +0100 Igor Mammedov wrote: > On Mon, 15 Feb 2021 09:56:19 +0100 > Vitaly Kuznetsov wrote: > > > Igor Mammedov writes: > > > > > On Fri, 12 Feb 2021 16:26:03 +0100 > > > Vitaly Kuznetsov wrote: > > > > > >> Vitaly Kuznetsov writes: > > >> > > >> > Igor Mammedov writes: > > >> > [...] > >(I think ppc does similar hing also) well scratch that off, I can't find PPC part anymore. Maybe I've confused that with something else. [...]
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Mon, 15 Feb 2021 09:53:50 +0100
Vitaly Kuznetsov wrote:
> Igor Mammedov writes:
>
> >> >
> >> > Please try reusing scratch CPU approach, see
> >> > kvm_arm_get_host_cpu_features()
> >> > for an example. You will very likely end up with simpler series,
> >> > compared to reinventing wheel.
> >>
> >> Even if I do that (and I serioulsy doubt it's going to be easier than
> >> just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200
> > it does a lot more then what you need, kvm_arm_create_scratch_host_vcpu()
> > which it uses will do the job and even that could be made smaller
> > for hv usecase.
> >
> >> lines long) this is not going to give us what we need to distinguish
> >> between
> >>
> >> 'hv-passthrough,hv-evmcs'
> >>
> >> and
> >>
> >> 'hv-passthrough'
> >>
> >> when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we
> >> don't want to enable it unless it was requested explicitly (former but
> >> not the later).
> > could you elaborate more on it, i.e. why do we need to distinguish and why
> > do we need evmcs without VMX if user asked for it (will it be usable)
> >
>
> We need to distinguish because that would be sane.
>
> Enlightened VMCS is an extension to VMX, it can't be used without
> it. Genuine Hyper-V doesn't have a knob for enabling and disabling it,
...
> That bein said, if
> guest CPU lacks VMX it is counter-productive to expose EVMCS. However,
> there is a problem with explicit enablement: what should
>
> 'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't
> sound sane to me.
based on above I'd error out is user asks for unsupported option
i.e. no VMX -> no hv-evmcs - if explicitly asked -> error out
if later on we find usecase for VMX=off + hv-evmcs=on,
we will be able to drop error without affecting existing users,
but not other way around.
> >> Moreover, instead of just adding two 'u64's we're now doing an ioctl
> >> which can fail, be subject to limits,... Creating and destroying a CPU
> >> is also slow. Sorry, I hardly see how this is better, maybe just from
> >> 'code purity' point of view.
> > readable and easy to maintain code is not a thing to neglect.
>
> Of couse, but 'scratch CPU' idea is not a good design decision, it is an
> ugly hack we should get rid of in ARM land, not try bringing it to other
> architectures. Generally, KVM should allow to query all its capabilities
> without the need to create a vCPU or, if not possible, we should create
> 'real' QEMU VCPUs and use one/all of the to query capabilities, avoiding
> 'scratch' because:
> - Creating and destroying a vCPU makes VM startup slower, much
> slower. E.g. for a single-CPU VM you're doubling the time required to
> create vCPUs!
> - vCPUs in KVM are quite memory consuming. Just 'struct kvm_vcpu_arch'
> was something like 12kb last time I looked at it.
>
> I have no clue why scratch vCPUs were implemented on ARM, however, I'd
> very much want us to avoid doing the same on x86. We do have use-cases
> where startup time and consumed memory is important. There is a point in
> limiting ioctls for security reasons (e.g. if I'm creating a single vCPU
> VM I may want to limit userspace process to one and only one
> KVM_CREATE_VCPU call).
it should be possible to reuse scratch VCPU (kvm file descriptor) as
the first CPU of VM, if there is a will/need, without creating unnecessary
overhead.
I don't like scratch CPU either but from my pov it's a lesser evil to
spawning custom parser every time someone fills like it.
> Now to the code you complain about. The 'hard to read and maintain' code
> is literaly this:
>
> +static void x86_hv_feature_set(Object *obj, bool value, int feature)
> +{
> +X86CPU *cpu = X86_CPU(obj);
> +
> +if (value) {
> +cpu->hyperv_features |= BIT(feature);
> +cpu->hyperv_features_on |= BIT(feature);
> +cpu->hyperv_features_off &= ~BIT(feature);
> +} else {
> +cpu->hyperv_features &= ~BIT(feature);
> +cpu->hyperv_features_on &= ~BIT(feature);
> +cpu->hyperv_features_off |= BIT(feature);
> +}
> +}
It's not just that code but the rest that uses above variables to
get final hyperv_features feature set. There is a lot of invariants
that are hidden in hv specific code that you put in hyperv kvm
specific part.
btw why can't we get supported hyperv_features in passthrough mode
during time we initialize KVM (without a vCPU)?
> I can add as many comments here as needed, however, I don't see what
> requires additional explanaition. We just want to know two things:
> - What's the 'effective' setting of the control
> - Was it explicitly enabled or disabled on the command line.
>
> Custom parsers are not new in QEMU and they're not going anywhere I
> believe. There are options with simple enablent and there are some with
> additional considerations. Trying to make CPU objects somewhat 'special'
> by forcing all options to be of type-1 (and thus crippling user
> exper
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Mon, 15 Feb 2021 09:56:19 +0100 Vitaly Kuznetsov wrote: > Igor Mammedov writes: > > > On Fri, 12 Feb 2021 16:26:03 +0100 > > Vitaly Kuznetsov wrote: > > > >> Vitaly Kuznetsov writes: > >> > >> > Igor Mammedov writes: > >> > > >> >> > >> >> Please try reusing scratch CPU approach, see > >> >> kvm_arm_get_host_cpu_features() > >> >> for an example. You will very likely end up with simpler series, > >> >> compared to reinventing wheel. > >> > > >> > Even if I do that (and I serioulsy doubt it's going to be easier than > >> > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 > >> > lines long) this is not going to give us what we need to distinguish > >> > between > >> > > >> > 'hv-passthrough,hv-evmcs' > >> > > >> > and > >> > > >> > 'hv-passthrough' > >> > > >> > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we > >> > don't want to enable it unless it was requested explicitly (former but > >> > not the later). > >> > >> ... and if for whatever reason we decide that this is also bad/not > >> needed, I can just drop patches 16-18 from the series (leaving > >> 'hv-passthrough,hv-feature=off' problem to better times). > > that's also an option, > > we would need to make sure that hv-passthrough is mutually exclusive > > with ''all'' other hv- properties to avoid above combination being > > ever (mis)used. > > That's an option to finally get these patches merged, not a good option > for end users. > > 'hv-passthrough,hv-feature' works today and it's useful. Should we drop > it? well, try suggested idea about using scratch CPU and it might get merged sooner. (it's not like I'm suggesting you to rewrite half of QEMU, just some of patches, which most likely would simplify series from my point of view and would be easier to maintain) > > 'hv-passthrough/hv-default' and 'hv-passthrough/hv-default,hv-evmcs' > should give us sane results. > > 'hv-passthrough,hv-feature=off' is convenient. > > Why droppping this all? To save 9 (nine) lines of code in the parser? it's doing what generic property parsing is capable off, provided you fish out hv-passthrough value in advance like arm/virt does (I think ppc does similar hing also), so I consider it as unnecessary code duplication/ complication and maintenance burden. If it were a hotfix during hard-freeze may be I'd agree (with promise to rework it later to something more palatable), but it's not, for patches in state they are now I'm not confident enough to ACK them.
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Mon, Feb 15, 2021 at 09:53:50AM +0100, Vitaly Kuznetsov wrote: > I have no clue why scratch vCPUs were implemented on ARM, however, I'd We don't have an ioctl like KVM_GET_SUPPORTED_CPUID, which operates on the KVM fd. Perhaps we should. Thanks, drew
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Fri, 12 Feb 2021 16:26:03 +0100 > Vitaly Kuznetsov wrote: > >> Vitaly Kuznetsov writes: >> >> > Igor Mammedov writes: >> > >> >> >> >> Please try reusing scratch CPU approach, see >> >> kvm_arm_get_host_cpu_features() >> >> for an example. You will very likely end up with simpler series, >> >> compared to reinventing wheel. >> > >> > Even if I do that (and I serioulsy doubt it's going to be easier than >> > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 >> > lines long) this is not going to give us what we need to distinguish >> > between >> > >> > 'hv-passthrough,hv-evmcs' >> > >> > and >> > >> > 'hv-passthrough' >> > >> > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we >> > don't want to enable it unless it was requested explicitly (former but >> > not the later). >> >> ... and if for whatever reason we decide that this is also bad/not >> needed, I can just drop patches 16-18 from the series (leaving >> 'hv-passthrough,hv-feature=off' problem to better times). > that's also an option, > we would need to make sure that hv-passthrough is mutually exclusive > with ''all'' other hv- properties to avoid above combination being > ever (mis)used. That's an option to finally get these patches merged, not a good option for end users. 'hv-passthrough,hv-feature' works today and it's useful. Should we drop it? 'hv-passthrough/hv-default' and 'hv-passthrough/hv-default,hv-evmcs' should give us sane results. 'hv-passthrough,hv-feature=off' is convenient. Why droppping this all? To save 9 (nine) lines of code in the parser? -- Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes:
>> >
>> > Please try reusing scratch CPU approach, see
>> > kvm_arm_get_host_cpu_features()
>> > for an example. You will very likely end up with simpler series,
>> > compared to reinventing wheel.
>>
>> Even if I do that (and I serioulsy doubt it's going to be easier than
>> just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200
> it does a lot more then what you need, kvm_arm_create_scratch_host_vcpu()
> which it uses will do the job and even that could be made smaller
> for hv usecase.
>
>> lines long) this is not going to give us what we need to distinguish
>> between
>>
>> 'hv-passthrough,hv-evmcs'
>>
>> and
>>
>> 'hv-passthrough'
>>
>> when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we
>> don't want to enable it unless it was requested explicitly (former but
>> not the later).
> could you elaborate more on it, i.e. why do we need to distinguish and why
> do we need evmcs without VMX if user asked for it (will it be usable)
>
We need to distinguish because that would be sane.
Enlightened VMCS is an extension to VMX, it can't be used without
it. Genuine Hyper-V doesn't have a knob for enabling and disabling it,
it comes with nesting (-ExposeVirtualizationExtensions $true). When we
create a default set of Hyper-V enlightenments (either 'hv-default' or
'hv-passthrough') we should be as close as possible to genuine Hyper-V
to not create unsupported Frankenstiens which can break with any Windows
update (because nobody tested these configurations). That bein said, if
guest CPU lacks VMX it is counter-productive to expose EVMCS. However,
there is a problem with explicit enablement: what should
'hv-passthrough,hv-evmcs' option do? Just silently drop EVMCS? Doesn't
sound sane to me.
>> Moreover, instead of just adding two 'u64's we're now doing an ioctl
>> which can fail, be subject to limits,... Creating and destroying a CPU
>> is also slow. Sorry, I hardly see how this is better, maybe just from
>> 'code purity' point of view.
> readable and easy to maintain code is not a thing to neglect.
Of couse, but 'scratch CPU' idea is not a good design decision, it is an
ugly hack we should get rid of in ARM land, not try bringing it to other
architectures. Generally, KVM should allow to query all its capabilities
without the need to create a vCPU or, if not possible, we should create
'real' QEMU VCPUs and use one/all of the to query capabilities, avoiding
'scratch' because:
- Creating and destroying a vCPU makes VM startup slower, much
slower. E.g. for a single-CPU VM you're doubling the time required to
create vCPUs!
- vCPUs in KVM are quite memory consuming. Just 'struct kvm_vcpu_arch'
was something like 12kb last time I looked at it.
I have no clue why scratch vCPUs were implemented on ARM, however, I'd
very much want us to avoid doing the same on x86. We do have use-cases
where startup time and consumed memory is important. There is a point in
limiting ioctls for security reasons (e.g. if I'm creating a single vCPU
VM I may want to limit userspace process to one and only one
KVM_CREATE_VCPU call).
Now to the code you complain about. The 'hard to read and maintain' code
is literaly this:
+static void x86_hv_feature_set(Object *obj, bool value, int feature)
+{
+X86CPU *cpu = X86_CPU(obj);
+
+if (value) {
+cpu->hyperv_features |= BIT(feature);
+cpu->hyperv_features_on |= BIT(feature);
+cpu->hyperv_features_off &= ~BIT(feature);
+} else {
+cpu->hyperv_features &= ~BIT(feature);
+cpu->hyperv_features_on &= ~BIT(feature);
+cpu->hyperv_features_off |= BIT(feature);
+}
+}
I can add as many comments here as needed, however, I don't see what
requires additional explanaition. We just want to know two things:
- What's the 'effective' setting of the control
- Was it explicitly enabled or disabled on the command line.
Custom parsers are not new in QEMU and they're not going anywhere I
believe. There are options with simple enablent and there are some with
additional considerations. Trying to make CPU objects somewhat 'special'
by forcing all options to be of type-1 (and thus crippling user
experience) is not the way to go IMHO. I'd very much like us to go in
another direction, make our option parser better so my very simple
use-case is covered 'out-of-the-box'.
--
Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Fri, 12 Feb 2021 16:26:03 +0100 Vitaly Kuznetsov wrote: > Vitaly Kuznetsov writes: > > > Igor Mammedov writes: > > > >> > >> Please try reusing scratch CPU approach, see > >> kvm_arm_get_host_cpu_features() > >> for an example. You will very likely end up with simpler series, > >> compared to reinventing wheel. > > > > Even if I do that (and I serioulsy doubt it's going to be easier than > > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 > > lines long) this is not going to give us what we need to distinguish > > between > > > > 'hv-passthrough,hv-evmcs' > > > > and > > > > 'hv-passthrough' > > > > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we > > don't want to enable it unless it was requested explicitly (former but > > not the later). > > ... and if for whatever reason we decide that this is also bad/not > needed, I can just drop patches 16-18 from the series (leaving > 'hv-passthrough,hv-feature=off' problem to better times). that's also an option, we would need to make sure that hv-passthrough is mutually exclusive with ''all'' other hv- properties to avoid above combination being ever (mis)used.
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Fri, 12 Feb 2021 16:19:24 +0100 Vitaly Kuznetsov wrote: > Igor Mammedov writes: > > > On Fri, 12 Feb 2021 09:45:52 +0100 > > Vitaly Kuznetsov wrote: > > > >> Igor Mammedov writes: > >> > >> > On Wed, 10 Feb 2021 17:40:28 +0100 > >> > Vitaly Kuznetsov wrote: > >> > > >> >> Sometimes we'd like to know which features were explicitly enabled and > >> >> which > >> >> were explicitly disabled on the command line. E.g. it seems logical to > >> >> handle > >> >> 'hv_passthrough,hv_feature=off' as "enable everything supported by the > >> >> host > >> >> except for hv_feature" but this doesn't seem to be possible with the > >> >> current > >> >> 'hyperv_features' bit array. Introduce > >> >> 'hv_features_on'/'hv_features_off' > >> >> add-ons and track explicit enablement/disablement there. > >> >> > >> >> Note, it doesn't seem to be possible to fill 'hyperv_features' array > >> >> during > >> >> CPU creation time when 'hv-passthrough' is specified and we're running > >> >> on > >> >> an older kernel without KVM_CAP_SYS_HYPERV_CPUID support. To get the > >> >> list > >> >> of the supported Hyper-V features we need to actually create KVM VCPU > >> >> and > >> >> this happens much later. > >> > > >> > seems to me that we are returning back to +-feat parsing, this time only > >> > for > >> > hyperv. > >> > I'm not sure I like it back, especially considering we are going to > >> > drop "-feat" priority for x86. > >> > > >> > now about impossible, see arm/kvm/virt, they create a 'sample' VCPU at > >> > KVM > >> > init time to probe for some CPU features in advance. You can use similar > >> > approach to prepare value for hyperv_features. > >> > > >> > >> KVM_CAP_SYS_HYPERV_CPUID is supported since 5.11 and eventually it'll > >> make it to all kernels we care about so I'd really like to avoid any > >> 'sample' CPUs for the time being. On/off parsing looks like a much > >> lesser evil. > > When minimum supported by QEMU kernel version gets there, you can remove > > scratch CPU in QEMU (if hyperv will remain its sole user). > > > > writing your own property parser like in this series, is possible too > > but it adds extra fields to track state and hard to follow logic. > > On top it adds a lot of churn by switching hv_ features to dynamic > > properties, which is not necessary if scratch CPU approach is used. > > > > Please try reusing scratch CPU approach, see > > kvm_arm_get_host_cpu_features() > > for an example. You will very likely end up with simpler series, > > compared to reinventing wheel. > > Even if I do that (and I serioulsy doubt it's going to be easier than > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 it does a lot more then what you need, kvm_arm_create_scratch_host_vcpu() which it uses will do the job and even that could be made smaller for hv usecase. > lines long) this is not going to give us what we need to distinguish > between > > 'hv-passthrough,hv-evmcs' > > and > > 'hv-passthrough' > > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we > don't want to enable it unless it was requested explicitly (former but > not the later). could you elaborate more on it, i.e. why do we need to distinguish and why do we need evmcs without VMX if user asked for it (will it be usable) > Moreover, instead of just adding two 'u64's we're now doing an ioctl > which can fail, be subject to limits,... Creating and destroying a CPU > is also slow. Sorry, I hardly see how this is better, maybe just from > 'code purity' point of view. readable and easy to maintain code is not a thing to neglect.
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Vitaly Kuznetsov writes: > Igor Mammedov writes: > >> >> Please try reusing scratch CPU approach, see >> kvm_arm_get_host_cpu_features() >> for an example. You will very likely end up with simpler series, >> compared to reinventing wheel. > > Even if I do that (and I serioulsy doubt it's going to be easier than > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 > lines long) this is not going to give us what we need to distinguish > between > > 'hv-passthrough,hv-evmcs' > > and > > 'hv-passthrough' > > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we > don't want to enable it unless it was requested explicitly (former but > not the later). ... and if for whatever reason we decide that this is also bad/not needed, I can just drop patches 16-18 from the series (leaving 'hv-passthrough,hv-feature=off' problem to better times). -- Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Fri, 12 Feb 2021 09:45:52 +0100 > Vitaly Kuznetsov wrote: > >> Igor Mammedov writes: >> >> > On Wed, 10 Feb 2021 17:40:28 +0100 >> > Vitaly Kuznetsov wrote: >> > >> >> Sometimes we'd like to know which features were explicitly enabled and >> >> which >> >> were explicitly disabled on the command line. E.g. it seems logical to >> >> handle >> >> 'hv_passthrough,hv_feature=off' as "enable everything supported by the >> >> host >> >> except for hv_feature" but this doesn't seem to be possible with the >> >> current >> >> 'hyperv_features' bit array. Introduce 'hv_features_on'/'hv_features_off' >> >> add-ons and track explicit enablement/disablement there. >> >> >> >> Note, it doesn't seem to be possible to fill 'hyperv_features' array >> >> during >> >> CPU creation time when 'hv-passthrough' is specified and we're running on >> >> an older kernel without KVM_CAP_SYS_HYPERV_CPUID support. To get the list >> >> of the supported Hyper-V features we need to actually create KVM VCPU and >> >> this happens much later. >> > >> > seems to me that we are returning back to +-feat parsing, this time only >> > for >> > hyperv. >> > I'm not sure I like it back, especially considering we are going to >> > drop "-feat" priority for x86. >> > >> > now about impossible, see arm/kvm/virt, they create a 'sample' VCPU at KVM >> > init time to probe for some CPU features in advance. You can use similar >> > approach to prepare value for hyperv_features. >> > >> >> KVM_CAP_SYS_HYPERV_CPUID is supported since 5.11 and eventually it'll >> make it to all kernels we care about so I'd really like to avoid any >> 'sample' CPUs for the time being. On/off parsing looks like a much >> lesser evil. > When minimum supported by QEMU kernel version gets there, you can remove > scratch CPU in QEMU (if hyperv will remain its sole user). > > writing your own property parser like in this series, is possible too > but it adds extra fields to track state and hard to follow logic. > On top it adds a lot of churn by switching hv_ features to dynamic > properties, which is not necessary if scratch CPU approach is used. > > Please try reusing scratch CPU approach, see > kvm_arm_get_host_cpu_features() > for an example. You will very likely end up with simpler series, > compared to reinventing wheel. Even if I do that (and I serioulsy doubt it's going to be easier than just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200 lines long) this is not going to give us what we need to distinguish between 'hv-passthrough,hv-evmcs' and 'hv-passthrough' when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we don't want to enable it unless it was requested explicitly (former but not the later). Moreover, instead of just adding two 'u64's we're now doing an ioctl which can fail, be subject to limits,... Creating and destroying a CPU is also slow. Sorry, I hardly see how this is better, maybe just from 'code purity' point of view. -- Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Fri, 12 Feb 2021 09:45:52 +0100 Vitaly Kuznetsov wrote: > Igor Mammedov writes: > > > On Wed, 10 Feb 2021 17:40:28 +0100 > > Vitaly Kuznetsov wrote: > > > >> Sometimes we'd like to know which features were explicitly enabled and > >> which > >> were explicitly disabled on the command line. E.g. it seems logical to > >> handle > >> 'hv_passthrough,hv_feature=off' as "enable everything supported by the host > >> except for hv_feature" but this doesn't seem to be possible with the > >> current > >> 'hyperv_features' bit array. Introduce 'hv_features_on'/'hv_features_off' > >> add-ons and track explicit enablement/disablement there. > >> > >> Note, it doesn't seem to be possible to fill 'hyperv_features' array during > >> CPU creation time when 'hv-passthrough' is specified and we're running on > >> an older kernel without KVM_CAP_SYS_HYPERV_CPUID support. To get the list > >> of the supported Hyper-V features we need to actually create KVM VCPU and > >> this happens much later. > > > > seems to me that we are returning back to +-feat parsing, this time only for > > hyperv. > > I'm not sure I like it back, especially considering we are going to > > drop "-feat" priority for x86. > > > > now about impossible, see arm/kvm/virt, they create a 'sample' VCPU at KVM > > init time to probe for some CPU features in advance. You can use similar > > approach to prepare value for hyperv_features. > > > > KVM_CAP_SYS_HYPERV_CPUID is supported since 5.11 and eventually it'll > make it to all kernels we care about so I'd really like to avoid any > 'sample' CPUs for the time being. On/off parsing looks like a much > lesser evil. When minimum supported by QEMU kernel version gets there, you can remove scratch CPU in QEMU (if hyperv will remain its sole user). writing your own property parser like in this series, is possible too but it adds extra fields to track state and hard to follow logic. On top it adds a lot of churn by switching hv_ features to dynamic properties, which is not necessary if scratch CPU approach is used. Please try reusing scratch CPU approach, see kvm_arm_get_host_cpu_features() for an example. You will very likely end up with simpler series, compared to reinventing wheel. in proto would look like: * kvm_init: hv_passthrough_cached = scratch_cpu->hyperv_features * property parsing time: x86_hv_passthrough_set() cpu->hyperv_features = hv_passthrough_cached all other features handled by generic property parsing, you don't have to do any special handling for them. * cpu_relalize() hv_expand() to check for dependencies, conflicts availability of features.
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
Igor Mammedov writes: > On Wed, 10 Feb 2021 17:40:28 +0100 > Vitaly Kuznetsov wrote: > >> Sometimes we'd like to know which features were explicitly enabled and which >> were explicitly disabled on the command line. E.g. it seems logical to handle >> 'hv_passthrough,hv_feature=off' as "enable everything supported by the host >> except for hv_feature" but this doesn't seem to be possible with the current >> 'hyperv_features' bit array. Introduce 'hv_features_on'/'hv_features_off' >> add-ons and track explicit enablement/disablement there. >> >> Note, it doesn't seem to be possible to fill 'hyperv_features' array during >> CPU creation time when 'hv-passthrough' is specified and we're running on >> an older kernel without KVM_CAP_SYS_HYPERV_CPUID support. To get the list >> of the supported Hyper-V features we need to actually create KVM VCPU and >> this happens much later. > > seems to me that we are returning back to +-feat parsing, this time only for > hyperv. > I'm not sure I like it back, especially considering we are going to > drop "-feat" priority for x86. > > now about impossible, see arm/kvm/virt, they create a 'sample' VCPU at KVM > init time to probe for some CPU features in advance. You can use similar > approach to prepare value for hyperv_features. > KVM_CAP_SYS_HYPERV_CPUID is supported since 5.11 and eventually it'll make it to all kernels we care about so I'd really like to avoid any 'sample' CPUs for the time being. On/off parsing looks like a much lesser evil. -- Vitaly
Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement
On Wed, 10 Feb 2021 17:40:28 +0100
Vitaly Kuznetsov wrote:
> Sometimes we'd like to know which features were explicitly enabled and which
> were explicitly disabled on the command line. E.g. it seems logical to handle
> 'hv_passthrough,hv_feature=off' as "enable everything supported by the host
> except for hv_feature" but this doesn't seem to be possible with the current
> 'hyperv_features' bit array. Introduce 'hv_features_on'/'hv_features_off'
> add-ons and track explicit enablement/disablement there.
>
> Note, it doesn't seem to be possible to fill 'hyperv_features' array during
> CPU creation time when 'hv-passthrough' is specified and we're running on
> an older kernel without KVM_CAP_SYS_HYPERV_CPUID support. To get the list
> of the supported Hyper-V features we need to actually create KVM VCPU and
> this happens much later.
seems to me that we are returning back to +-feat parsing, this time only for
hyperv.
I'm not sure I like it back, especially considering we are going to
drop "-feat" priority for x86.
now about impossible, see arm/kvm/virt, they create a 'sample' VCPU at KVM
init time to probe for some CPU features in advance. You can use similar
approach to prepare value for hyperv_features.
>
> No functional change intended.
>
> Signed-off-by: Vitaly Kuznetsov
> ---
> target/i386/cpu.c | 237 --
> target/i386/cpu.h | 2 +
> 2 files changed, 209 insertions(+), 30 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c4e8863c7ca0..e8a004c39d04 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4553,6 +4553,178 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> Visitor *v, const char *name,
> cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> }
>
> +static bool x86_hv_feature_get(Object *obj, int feature)
> +{
> +X86CPU *cpu = X86_CPU(obj);
> +
> +return cpu->hyperv_features & BIT(feature);
> +}
> +
> +static void x86_hv_feature_set(Object *obj, bool value, int feature)
> +{
> +X86CPU *cpu = X86_CPU(obj);
> +
> +if (value) {
> +cpu->hyperv_features |= BIT(feature);
> +cpu->hyperv_features_on |= BIT(feature);
> +cpu->hyperv_features_off &= ~BIT(feature);
> +} else {
> +cpu->hyperv_features &= ~BIT(feature);
> +cpu->hyperv_features_on &= ~BIT(feature);
> +cpu->hyperv_features_off |= BIT(feature);
> +}
> +}
> +
> +static bool x86_hv_relaxed_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_RELAXED);
> +}
> +
> +static void x86_hv_relaxed_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_RELAXED);
> +}
> +
> +static bool x86_hv_vapic_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_VAPIC);
> +}
> +
> +static void x86_hv_vapic_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_VAPIC);
> +}
> +
> +static bool x86_hv_time_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_TIME);
> +}
> +
> +static void x86_hv_time_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_TIME);
> +}
> +
> +static bool x86_hv_crash_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_CRASH);
> +}
> +
> +static void x86_hv_crash_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_CRASH);
> +}
> +
> +static bool x86_hv_reset_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_RESET);
> +}
> +
> +static void x86_hv_reset_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_RESET);
> +}
> +
> +static bool x86_hv_vpindex_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_VPINDEX);
> +}
> +
> +static void x86_hv_vpindex_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_VPINDEX);
> +}
> +
> +static bool x86_hv_runtime_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_RUNTIME);
> +}
> +
> +static void x86_hv_runtime_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_RUNTIME);
> +}
> +
> +static bool x86_hv_synic_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_SYNIC);
> +}
> +
> +static void x86_hv_synic_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_SYNIC);
> +}
> +
> +static bool x86_hv_stimer_get(Object *obj, Error **errp)
> +{
> +return x86_hv_feature_get(obj, HYPERV_FEAT_STIMER);
> +}
> +
> +static void x86_hv_stimer_set(Object *obj, bool value, Error **errp)
> +{
> +x86_hv_feature_set(obj, value, HYPERV_FEAT_STIMER);
> +}
> +
> +static bool x86_hv_frequencies_get(Object *obj, Error **errp)
>
