| Message ID | 1361754189-29809-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote: > - since hyperv_* helper functions are used only in target-i386/kvm.c > move them there as static helpers > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Requestd-By: Eduardo Habkost <ehabkost@redhat.com> I wonder if we could do this later, to make review and testing easier. We could simply keep the hv_* handling inside parse_featurestr() by now. I don't think this would block any of the CPU hotplug or CPU model probing/compatibility/reliability work we're doing. I mean: * I don't expect hv-* to appear on a machine-type compat_props array in the near feature. * I don't expect people to need to set per-CPU hv-* properties on device_add for CPU hotplug. So we could keep them as special cases on parse_featurestr(), and convert them to per-CPU properties only after we have the subclasses and CPU hotplug working. Considering that -cpu options will be translated to global properties, it will be trivial to keep compatibility with existing behavior of "-cpu hv_*=..." once we change them from static variables to per-CPU fields. > --- > target-i386/Makefile.objs | 2 +- > target-i386/cpu.c | 16 +++++++--- > target-i386/cpu.h | 7 +++++ > target-i386/hyperv.c | 64 --------------------------------------------- > target-i386/hyperv.h | 45 ------------------------------- > target-i386/kvm.c | 36 ++++++++++++++++++------- > 6 files changed, 45 insertions(+), 125 deletions(-) > delete mode 100644 target-i386/hyperv.c > delete mode 100644 target-i386/hyperv.h > [...]
On Tue, 26 Feb 2013 11:50:23 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote: > > - since hyperv_* helper functions are used only in target-i386/kvm.c > > move them there as static helpers > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Requestd-By: Eduardo Habkost <ehabkost@redhat.com> > > I wonder if we could do this later, to make review and testing easier. > We could simply keep the hv_* handling inside parse_featurestr() by now. > I don't think this would block any of the CPU hotplug or CPU model > probing/compatibility/reliability work we're doing. I mean: device_add from CPU hot-add POV would be usable once we have all features accepted on -cpu converted to properties + sub-classes. > > * I don't expect hv-* to appear on a machine-type compat_props array in > the near feature. > * I don't expect people to need to set per-CPU hv-* properties on > device_add for CPU hotplug. > > So we could keep them as special cases on parse_featurestr(), and > convert them to per-CPU properties only after we have the subclasses and > CPU hotplug working. it won't be a consistent interface, where user who has "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line would have to use "device_add XXX,foo1=true,foo2=true" manually excluding options from device_add, i.e. it propagates special casing to users as well. And when hv_ are moved to per-CPU fields, it might break users since they will still exclude some options. > > Considering that -cpu options will be translated to global properties, > it will be trivial to keep compatibility with existing behavior of "-cpu > hv_*=..." once we change them from static variables to per-CPU fields. Global properties would just allow not to specify foo1,foo2 on device_add from hot-plug POV. If this and following patch are to complex we could fallback to alternative from v6 for hv_* features, which produce the same external property interface just with different internal approach. > > > --- > > target-i386/Makefile.objs | 2 +- > > target-i386/cpu.c | 16 +++++++--- > > target-i386/cpu.h | 7 +++++ > > target-i386/hyperv.c | 64 > > --------------------------------------------- target-i386/hyperv.h > > | 45 ------------------------------- target-i386/kvm.c | 36 > > ++++++++++++++++++------- 6 files changed, 45 insertions(+), 125 > > deletions(-) delete mode 100644 target-i386/hyperv.c > > delete mode 100644 target-i386/hyperv.h > > > [...] >
On Tue, Feb 26, 2013 at 04:42:17PM +0100, Igor Mammedov wrote: > On Tue, 26 Feb 2013 11:50:23 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote: > > > - since hyperv_* helper functions are used only in target-i386/kvm.c > > > move them there as static helpers > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Requestd-By: Eduardo Habkost <ehabkost@redhat.com> > > > > I wonder if we could do this later, to make review and testing easier. > > We could simply keep the hv_* handling inside parse_featurestr() by now. > > I don't think this would block any of the CPU hotplug or CPU model > > probing/compatibility/reliability work we're doing. I mean: > device_add from CPU hot-add POV would be usable once we have all features > accepted on -cpu converted to properties + sub-classes. If we keep hv_* as special cases inside parse_featurestr() (the way they already are), CPU hotplug should still work perfectly. I don't see why it would block it. > > > > > * I don't expect hv-* to appear on a machine-type compat_props array in > > the near feature. > > * I don't expect people to need to set per-CPU hv-* properties on > > device_add for CPU hotplug. > > > > So we could keep them as special cases on parse_featurestr(), and > > convert them to per-CPU properties only after we have the subclasses and > > CPU hotplug working. > it won't be a consistent interface, where user who has > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > would have to use "device_add XXX,foo1=true,foo2=true" manually excluding > options from device_add, i.e. it propagates special casing to users as > well. And when hv_ are moved to per-CPU fields, it might break users since > they will still exclude some options. Won't -cpu/parse_featurestr() simply set global properties? In this case, the common case would be to call "device_add XXX" with no extra options at all, so there's no option to be excluded and no special case to care about. > > > > Considering that -cpu options will be translated to global properties, > > it will be trivial to keep compatibility with existing behavior of "-cpu > > hv_*=..." once we change them from static variables to per-CPU fields. > Global properties would just allow not to specify foo1,foo2 on > device_add from hot-plug POV. > > If this and following patch are to complex we could fallback to alternative > from v6 for hv_* features, which produce the same external property interface > just with different internal approach. No, v6 exposes completely different (and unexpected) semantics. It exposes a per-CPU property that affects all CPU objects when it gets changed. > > > > > > --- > > > target-i386/Makefile.objs | 2 +- > > > target-i386/cpu.c | 16 +++++++--- > > > target-i386/cpu.h | 7 +++++ > > > target-i386/hyperv.c | 64 > > > --------------------------------------------- target-i386/hyperv.h > > > | 45 ------------------------------- target-i386/kvm.c | 36 > > > ++++++++++++++++++------- 6 files changed, 45 insertions(+), 125 > > > deletions(-) delete mode 100644 target-i386/hyperv.c > > > delete mode 100644 target-i386/hyperv.h > > > > > [...] > > > >
On Tue, 26 Feb 2013 13:01:12 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 26, 2013 at 04:42:17PM +0100, Igor Mammedov wrote: > > On Tue, 26 Feb 2013 11:50:23 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote: > > > > - since hyperv_* helper functions are used only in target-i386/kvm.c > > > > move them there as static helpers > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Requestd-By: Eduardo Habkost <ehabkost@redhat.com> > > > > > > I wonder if we could do this later, to make review and testing easier. > > > We could simply keep the hv_* handling inside parse_featurestr() by now. > > > I don't think this would block any of the CPU hotplug or CPU model > > > probing/compatibility/reliability work we're doing. I mean: > > device_add from CPU hot-add POV would be usable once we have all features > > accepted on -cpu converted to properties + sub-classes. > > If we keep hv_* as special cases inside parse_featurestr() (the way they > already are), CPU hotplug should still work perfectly. I don't see why > it would block it. > > > > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props array in > > > the near feature. > > > * I don't expect people to need to set per-CPU hv-* properties on > > > device_add for CPU hotplug. > > > > > > So we could keep them as special cases on parse_featurestr(), and > > > convert them to per-CPU properties only after we have the subclasses and > > > CPU hotplug working. > > it won't be a consistent interface, where user who has > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > would have to use "device_add XXX,foo1=true,foo2=true" manually excluding > > options from device_add, i.e. it propagates special casing to users as > > well. And when hv_ are moved to per-CPU fields, it might break users since > > they will still exclude some options. > > Won't -cpu/parse_featurestr() simply set global properties? In this > case, the common case would be to call "device_add XXX" with no extra > options at all, so there's no option to be excluded and no special case > to care about. That is if global properties will made to 1.5 which I highly doubt taking in account how fast patches are reviewed and accepted. Otherwise release would be broken. > > > > > > > > Considering that -cpu options will be translated to global properties, > > > it will be trivial to keep compatibility with existing behavior of "-cpu > > > hv_*=..." once we change them from static variables to per-CPU fields. > > Global properties would just allow not to specify foo1,foo2 on > > device_add from hot-plug POV. > > > > If this and following patch are to complex we could fallback to > > alternative from v6 for hv_* features, which produce the same external > > property interface just with different internal approach. > > No, v6 exposes completely different (and unexpected) semantics. It > exposes a per-CPU property that affects all CPU objects when it gets > changed. Do you know any guest that will work with mixed CPUs that have and doesn't have hv_* set at the same time? > > > > > > > > > --- > > > > target-i386/Makefile.objs | 2 +- > > > > target-i386/cpu.c | 16 +++++++--- > > > > target-i386/cpu.h | 7 +++++ > > > > target-i386/hyperv.c | 64 > > > > --------------------------------------------- target-i386/hyperv.h > > > > | 45 ------------------------------- target-i386/kvm.c | > > > > 36 ++++++++++++++++++------- 6 files changed, 45 insertions(+), 125 > > > > deletions(-) delete mode 100644 target-i386/hyperv.c > > > > delete mode 100644 target-i386/hyperv.h > > > > > > > [...] > > > > > > > >
On Tue, Feb 26, 2013 at 05:12:21PM +0100, Igor Mammedov wrote: > On Tue, 26 Feb 2013 13:01:12 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Feb 26, 2013 at 04:42:17PM +0100, Igor Mammedov wrote: > > > On Tue, 26 Feb 2013 11:50:23 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote: > > > > > - since hyperv_* helper functions are used only in target-i386/kvm.c > > > > > move them there as static helpers > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > Requestd-By: Eduardo Habkost <ehabkost@redhat.com> > > > > > > > > I wonder if we could do this later, to make review and testing easier. > > > > We could simply keep the hv_* handling inside parse_featurestr() by now. > > > > I don't think this would block any of the CPU hotplug or CPU model > > > > probing/compatibility/reliability work we're doing. I mean: > > > device_add from CPU hot-add POV would be usable once we have all features > > > accepted on -cpu converted to properties + sub-classes. > > > > If we keep hv_* as special cases inside parse_featurestr() (the way they > > already are), CPU hotplug should still work perfectly. I don't see why > > it would block it. > > > > > > > > > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props array in > > > > the near feature. > > > > * I don't expect people to need to set per-CPU hv-* properties on > > > > device_add for CPU hotplug. > > > > > > > > So we could keep them as special cases on parse_featurestr(), and > > > > convert them to per-CPU properties only after we have the subclasses and > > > > CPU hotplug working. > > > it won't be a consistent interface, where user who has > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > > would have to use "device_add XXX,foo1=true,foo2=true" manually excluding > > > options from device_add, i.e. it propagates special casing to users as > > > well. And when hv_ are moved to per-CPU fields, it might break users since > > > they will still exclude some options. > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > case, the common case would be to call "device_add XXX" with no extra > > options at all, so there's no option to be excluded and no special case > > to care about. > That is if global properties will made to 1.5 which I highly doubt taking in > account how fast patches are reviewed and accepted. Otherwise release would > be broken. IMO it _has_ to make 1.5 and is a requirement to make device_add usable for CPU hotplug. Otherwise we would have to change the behavior of -cpu + device_add in an incompatible way. > > > > > > > > > > > > > Considering that -cpu options will be translated to global properties, > > > > it will be trivial to keep compatibility with existing behavior of "-cpu > > > > hv_*=..." once we change them from static variables to per-CPU fields. > > > Global properties would just allow not to specify foo1,foo2 on > > > device_add from hot-plug POV. > > > > > > If this and following patch are to complex we could fallback to > > > alternative from v6 for hv_* features, which produce the same external > > > property interface just with different internal approach. > > > > No, v6 exposes completely different (and unexpected) semantics. It > > exposes a per-CPU property that affects all CPU objects when it gets > > changed. > Do you know any guest that will work with mixed CPUs that have and doesn't > have hv_* set at the same time? I don't know and I don't care. But in either case you are introducing a per-CPU QOM property for it. If it is a per-CPU QOM property, it just makes sense that it will affect only the CPU object being changed. What I am proposing is to make it a per-CPU QOM property only after we make it really per-CPU, so we don't introduce weird externally-visible property semantics that are likely to change in the near future. > > > > > > > > > > > > > --- > > > > > target-i386/Makefile.objs | 2 +- > > > > > target-i386/cpu.c | 16 +++++++--- > > > > > target-i386/cpu.h | 7 +++++ > > > > > target-i386/hyperv.c | 64 > > > > > --------------------------------------------- target-i386/hyperv.h > > > > > | 45 ------------------------------- target-i386/kvm.c | > > > > > 36 ++++++++++++++++++------- 6 files changed, 45 insertions(+), 125 > > > > > deletions(-) delete mode 100644 target-i386/hyperv.c > > > > > delete mode 100644 target-i386/hyperv.h > > > > > > > > > [...] > > > > > > > > > > > > >
On Tue, 26 Feb 2013 13:22:22 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 26, 2013 at 05:12:21PM +0100, Igor Mammedov wrote: > > On Tue, 26 Feb 2013 13:01:12 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Feb 26, 2013 at 04:42:17PM +0100, Igor Mammedov wrote: > > > > On Tue, 26 Feb 2013 11:50:23 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote: > > > > > > - since hyperv_* helper functions are used only in > > > > > > target-i386/kvm.c move them there as static helpers > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Requestd-By: Eduardo Habkost <ehabkost@redhat.com> > > > > > > > > > > I wonder if we could do this later, to make review and testing > > > > > easier. We could simply keep the hv_* handling inside > > > > > parse_featurestr() by now. I don't think this would block any of > > > > > the CPU hotplug or CPU model probing/compatibility/reliability work > > > > > we're doing. I mean: > > > > device_add from CPU hot-add POV would be usable once we have all > > > > features accepted on -cpu converted to properties + sub-classes. > > > > > > If we keep hv_* as special cases inside parse_featurestr() (the way they > > > already are), CPU hotplug should still work perfectly. I don't see why > > > it would block it. > > > > > > > > > > > > > > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props > > > > > array in the near feature. > > > > > * I don't expect people to need to set per-CPU hv-* properties on > > > > > device_add for CPU hotplug. > > > > > > > > > > So we could keep them as special cases on parse_featurestr(), and > > > > > convert them to per-CPU properties only after we have the > > > > > subclasses and CPU hotplug working. > > > > it won't be a consistent interface, where user who has > > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > > > would have to use "device_add XXX,foo1=true,foo2=true" manually > > > > excluding options from device_add, i.e. it propagates special casing > > > > to users as well. And when hv_ are moved to per-CPU fields, it might > > > > break users since they will still exclude some options. > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > case, the common case would be to call "device_add XXX" with no extra > > > options at all, so there's no option to be excluded and no special case > > > to care about. > > That is if global properties will made to 1.5 which I highly doubt > > taking in account how fast patches are reviewed and accepted. Otherwise > > release would be broken. > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > + device_add in an incompatible way. if all -cpu features are converted to static properties, we do not have to have global properties working. In absence of 'global properties', user will have to use the same properties at device_add that was specified at -cpu. And introduction of global properties won't regress it, it will just allow to use device_add without features specified on -cpu > > > > > > > > > > > > > > > > > > > > Considering that -cpu options will be translated to global > > > > > properties, it will be trivial to keep compatibility with existing > > > > > behavior of "-cpu hv_*=..." once we change them from static > > > > > variables to per-CPU fields. > > > > Global properties would just allow not to specify foo1,foo2 on > > > > device_add from hot-plug POV. > > > > > > > > If this and following patch are to complex we could fallback to > > > > alternative from v6 for hv_* features, which produce the same external > > > > property interface just with different internal approach. > > > > > > No, v6 exposes completely different (and unexpected) semantics. It > > > exposes a per-CPU property that affects all CPU objects when it gets > > > changed. > > Do you know any guest that will work with mixed CPUs that have and doesn't > > have hv_* set at the same time? > > I don't know and I don't care. But in either case you are introducing a > per-CPU QOM property for it. If it is a per-CPU QOM property, it just > makes sense that it will affect only the CPU object being changed. > > What I am proposing is to make it a per-CPU QOM property only after we > make it really per-CPU, so we don't introduce weird externally-visible > property semantics that are likely to change in the near future. Well this patch and next do it per-CPU as you asked, so no weird property semantics is introduced and no special-casing of anything is required. > > > > > > > > > > > > > > > > > > > --- > > > > > > target-i386/Makefile.objs | 2 +- > > > > > > target-i386/cpu.c | 16 +++++++--- > > > > > > target-i386/cpu.h | 7 +++++ > > > > > > target-i386/hyperv.c | 64 > > > > > > --------------------------------------------- target-i386/hyperv.h > > > > > > | 45 ------------------------------- target-i386/kvm.c | > > > > > > 36 ++++++++++++++++++------- 6 files changed, 45 insertions(+), > > > > > > 125 deletions(-) delete mode 100644 target-i386/hyperv.c > > > > > > delete mode 100644 target-i386/hyperv.h > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > >
On Tue, Feb 26, 2013 at 05:39:02PM +0100, Igor Mammedov wrote: [...] > > > > > > * I don't expect hv-* to appear on a machine-type compat_props > > > > > > array in the near feature. > > > > > > * I don't expect people to need to set per-CPU hv-* properties on > > > > > > device_add for CPU hotplug. > > > > > > > > > > > > So we could keep them as special cases on parse_featurestr(), and > > > > > > convert them to per-CPU properties only after we have the > > > > > > subclasses and CPU hotplug working. > > > > > it won't be a consistent interface, where user who has > > > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > > > > would have to use "device_add XXX,foo1=true,foo2=true" manually > > > > > excluding options from device_add, i.e. it propagates special casing > > > > > to users as well. And when hv_ are moved to per-CPU fields, it might > > > > > break users since they will still exclude some options. > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > case, the common case would be to call "device_add XXX" with no extra > > > > options at all, so there's no option to be excluded and no special case > > > > to care about. > > > That is if global properties will made to 1.5 which I highly doubt > > > taking in account how fast patches are reviewed and accepted. Otherwise > > > release would be broken. > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > + device_add in an incompatible way. > if all -cpu features are converted to static properties, we do not have to > have global properties working. In absence of 'global properties', user will > have to use the same properties at device_add that was specified at -cpu. > And introduction of global properties won't regress it, it will just allow to > use device_add without features specified on -cpu Strictly, we do not have to, but changing -cpu to set global properties only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed by "device_add XXX" in an incompatible way. So if our long-term plan is to make "-cpu" change the global-properties/defaults, we need to do it since the beginning. > > > > > > > > > > > > Considering that -cpu options will be translated to global > > > > > > properties, it will be trivial to keep compatibility with existing > > > > > > behavior of "-cpu hv_*=..." once we change them from static > > > > > > variables to per-CPU fields. > > > > > Global properties would just allow not to specify foo1,foo2 on > > > > > device_add from hot-plug POV. > > > > > > > > > > If this and following patch are to complex we could fallback to > > > > > alternative from v6 for hv_* features, which produce the same external > > > > > property interface just with different internal approach. > > > > > > > > No, v6 exposes completely different (and unexpected) semantics. It > > > > exposes a per-CPU property that affects all CPU objects when it gets > > > > changed. > > > Do you know any guest that will work with mixed CPUs that have and doesn't > > > have hv_* set at the same time? > > > > I don't know and I don't care. But in either case you are introducing a > > per-CPU QOM property for it. If it is a per-CPU QOM property, it just > > makes sense that it will affect only the CPU object being changed. > > > > What I am proposing is to make it a per-CPU QOM property only after we > > make it really per-CPU, so we don't introduce weird externally-visible > > property semantics that are likely to change in the near future. > Well this patch and next do it per-CPU as you asked, so no weird property > semantics is introduced and no special-casing of anything is required. Yes, this patch does what I asked for, and I am not really against it. :-) I am just wondering if we could do these changes later to make review and testing easier.
On Tue, 26 Feb 2013 14:06:05 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 26, 2013 at 05:39:02PM +0100, Igor Mammedov wrote: > [...] > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props > > > > > > > array in the near feature. > > > > > > > * I don't expect people to need to set per-CPU hv-* properties > > > > > > > on device_add for CPU hotplug. > > > > > > > > > > > > > > So we could keep them as special cases on parse_featurestr(), > > > > > > > and convert them to per-CPU properties only after we have the > > > > > > > subclasses and CPU hotplug working. > > > > > > it won't be a consistent interface, where user who has > > > > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > > > > > would have to use "device_add XXX,foo1=true,foo2=true" manually > > > > > > excluding options from device_add, i.e. it propagates special > > > > > > casing to users as well. And when hv_ are moved to per-CPU > > > > > > fields, it might break users since they will still exclude some > > > > > > options. > > > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > > case, the common case would be to call "device_add XXX" with no > > > > > extra options at all, so there's no option to be excluded and no > > > > > special case to care about. > > > > That is if global properties will made to 1.5 which I highly doubt > > > > taking in account how fast patches are reviewed and accepted. > > > > Otherwise release would be broken. > > > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > > + device_add in an incompatible way. > > if all -cpu features are converted to static properties, we do not have to > > have global properties working. In absence of 'global properties', user > > will have to use the same properties at device_add that was specified at > > -cpu. And introduction of global properties won't regress it, it will > > just allow to use device_add without features specified on -cpu > > Strictly, we do not have to, but changing -cpu to set global properties > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed > by "device_add XXX" in an incompatible way. So if our long-term plan is Could you explain how ^^^ it will be incompatible, pls? > to make "-cpu" change the global-properties/defaults, we need to do it > since the beginning. > > > > > > > > > > > > > > > Considering that -cpu options will be translated to global > > > > > > > properties, it will be trivial to keep compatibility with > > > > > > > existing behavior of "-cpu hv_*=..." once we change them from > > > > > > > static variables to per-CPU fields. > > > > > > Global properties would just allow not to specify foo1,foo2 on > > > > > > device_add from hot-plug POV. > > > > > > > > > > > > If this and following patch are to complex we could fallback to > > > > > > alternative from v6 for hv_* features, which produce the same > > > > > > external property interface just with different internal approach. > > > > > > > > > > No, v6 exposes completely different (and unexpected) semantics. It > > > > > exposes a per-CPU property that affects all CPU objects when it gets > > > > > changed. > > > > Do you know any guest that will work with mixed CPUs that have and > > > > doesn't have hv_* set at the same time? > > > > > > I don't know and I don't care. But in either case you are introducing a > > > per-CPU QOM property for it. If it is a per-CPU QOM property, it just > > > makes sense that it will affect only the CPU object being changed. > > > > > > What I am proposing is to make it a per-CPU QOM property only after we > > > make it really per-CPU, so we don't introduce weird externally-visible > > > property semantics that are likely to change in the near future. > > Well this patch and next do it per-CPU as you asked, so no weird property > > semantics is introduced and no special-casing of anything is required. > > Yes, this patch does what I asked for, and I am not really against it. > :-) > > I am just wondering if we could do these changes later to make review > and testing easier. >
On Tue, Feb 26, 2013 at 06:10:25PM +0100, Igor Mammedov wrote: > On Tue, 26 Feb 2013 14:06:05 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Feb 26, 2013 at 05:39:02PM +0100, Igor Mammedov wrote: > > [...] > > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props > > > > > > > > array in the near feature. > > > > > > > > * I don't expect people to need to set per-CPU hv-* properties > > > > > > > > on device_add for CPU hotplug. > > > > > > > > > > > > > > > > So we could keep them as special cases on parse_featurestr(), > > > > > > > > and convert them to per-CPU properties only after we have the > > > > > > > > subclasses and CPU hotplug working. > > > > > > > it won't be a consistent interface, where user who has > > > > > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > > > > > > would have to use "device_add XXX,foo1=true,foo2=true" manually > > > > > > > excluding options from device_add, i.e. it propagates special > > > > > > > casing to users as well. And when hv_ are moved to per-CPU > > > > > > > fields, it might break users since they will still exclude some > > > > > > > options. > > > > > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > > > case, the common case would be to call "device_add XXX" with no > > > > > > extra options at all, so there's no option to be excluded and no > > > > > > special case to care about. > > > > > That is if global properties will made to 1.5 which I highly doubt > > > > > taking in account how fast patches are reviewed and accepted. > > > > > Otherwise release would be broken. > > > > > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > > > + device_add in an incompatible way. > > > if all -cpu features are converted to static properties, we do not have to > > > have global properties working. In absence of 'global properties', user > > > will have to use the same properties at device_add that was specified at > > > -cpu. And introduction of global properties won't regress it, it will > > > just allow to use device_add without features specified on -cpu > > > > Strictly, we do not have to, but changing -cpu to set global properties > > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed > > by "device_add XXX" in an incompatible way. So if our long-term plan is > Could you explain how ^^^ it will be incompatible, pls? Suppose that "foo" defaults to 0, and we run: "-cpu XXX,foo=1", followed by "device_add XXX". Without globals/defaults set by -cpu, the above will create a new CPU with foo=0. With globals/defaults set by -cpu, the above will create a new CPU with foo=1. If I recall correctly, we agreed that the latter is the behavior we wanted (because it is simpler for users, matches the fact that "-cpu" already affects multiple CPU devices [it already affects all the CPUs created on startup], and is the most common use-case [creating CPUs that look basically the same]).
On Tue, 26 Feb 2013 14:21:22 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 26, 2013 at 06:10:25PM +0100, Igor Mammedov wrote: > > On Tue, 26 Feb 2013 14:06:05 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Feb 26, 2013 at 05:39:02PM +0100, Igor Mammedov wrote: > > > [...] > > > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props > > > > > > > > > array in the near feature. > > > > > > > > > * I don't expect people to need to set per-CPU hv-* properties > > > > > > > > > on device_add for CPU hotplug. > > > > > > > > > > > > > > > > > > So we could keep them as special cases on parse_featurestr(), > > > > > > > > > and convert them to per-CPU properties only after we have the > > > > > > > > > subclasses and CPU hotplug working. > > > > > > > > it won't be a consistent interface, where user who has > > > > > > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line > > > > > > > > would have to use "device_add XXX,foo1=true,foo2=true" manually > > > > > > > > excluding options from device_add, i.e. it propagates special > > > > > > > > casing to users as well. And when hv_ are moved to per-CPU > > > > > > > > fields, it might break users since they will still exclude some > > > > > > > > options. > > > > > > > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > > > > case, the common case would be to call "device_add XXX" with no > > > > > > > extra options at all, so there's no option to be excluded and no > > > > > > > special case to care about. > > > > > > That is if global properties will made to 1.5 which I highly doubt > > > > > > taking in account how fast patches are reviewed and accepted. > > > > > > Otherwise release would be broken. > > > > > > > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > > > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > > > > + device_add in an incompatible way. > > > > if all -cpu features are converted to static properties, we do not have to > > > > have global properties working. In absence of 'global properties', user > > > > will have to use the same properties at device_add that was specified at > > > > -cpu. And introduction of global properties won't regress it, it will > > > > just allow to use device_add without features specified on -cpu > > > > > > Strictly, we do not have to, but changing -cpu to set global properties > > > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed > > > by "device_add XXX" in an incompatible way. So if our long-term plan is > > Could you explain how ^^^ it will be incompatible, pls? > > Suppose that "foo" defaults to 0, and we run: "-cpu XXX,foo=1", followed > by "device_add XXX". > > Without globals/defaults set by -cpu, the above will create a new CPU > with foo=0. > > With globals/defaults set by -cpu, the above will create a new CPU with > foo=1. > > If I recall correctly, we agreed that the latter is the behavior we > wanted (because it is simpler for users, matches the fact that "-cpu" > already affects multiple CPU devices [it already affects all the CPUs > created on startup], and is the most common use-case [creating CPUs that > look basically the same]). Yes, that is the goal. I wouldn't say incompatible if user will start QEMU with "-cpu XXX,foo=1" and then use "device_add XXX,foo=1". That's a strict minimum that would work for hot-plug. Plain "device_add XXX" is an invalid in this case since it won't produce the same CPU. So later on top of "-cpu XXX,foo=1" + "device_add XXX,foo=1" we add up conversion of -cpu to global properties it shouldn't break anything, only add new option to create the same CPU usin "device_add XXX", users will still be able to use "device_add XXX,foo=1" if desired. I hope that -cpu => global properties will make it in 1.5, but it is not must have show-stopper for hot-plug if it misses it. > > -- > Eduardo >
On Tue, Feb 26, 2013 at 08:03:16PM +0100, Igor Mammedov wrote: [...] > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > > > > > case, the common case would be to call "device_add XXX" with no > > > > > > > > extra options at all, so there's no option to be excluded and no > > > > > > > > special case to care about. > > > > > > > That is if global properties will made to 1.5 which I highly doubt > > > > > > > taking in account how fast patches are reviewed and accepted. > > > > > > > Otherwise release would be broken. > > > > > > > > > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > > > > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > > > > > + device_add in an incompatible way. > > > > > if all -cpu features are converted to static properties, we do not have to > > > > > have global properties working. In absence of 'global properties', user > > > > > will have to use the same properties at device_add that was specified at > > > > > -cpu. And introduction of global properties won't regress it, it will > > > > > just allow to use device_add without features specified on -cpu > > > > > > > > Strictly, we do not have to, but changing -cpu to set global properties > > > > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed > > > > by "device_add XXX" in an incompatible way. So if our long-term plan is > > > Could you explain how ^^^ it will be incompatible, pls? > > > > Suppose that "foo" defaults to 0, and we run: "-cpu XXX,foo=1", followed > > by "device_add XXX". > > > > Without globals/defaults set by -cpu, the above will create a new CPU > > with foo=0. > > > > With globals/defaults set by -cpu, the above will create a new CPU with > > foo=1. > > > > If I recall correctly, we agreed that the latter is the behavior we > > wanted (because it is simpler for users, matches the fact that "-cpu" > > already affects multiple CPU devices [it already affects all the CPUs > > created on startup], and is the most common use-case [creating CPUs that > > look basically the same]). > Yes, that is the goal. I wouldn't say incompatible if user will start > QEMU with "-cpu XXX,foo=1" and then use "device_add XXX,foo=1". That's a > strict minimum that would work for hot-plug. Plain "device_add XXX" is an > invalid in this case since it won't produce the same CPU. > So later on top of "-cpu XXX,foo=1" + "device_add XXX,foo=1" we add up > conversion of -cpu to global properties it shouldn't break anything, only add > new option to create the same CPU usin "device_add XXX", users will still be > able to use "device_add XXX,foo=1" if desired. > > I hope that -cpu => global properties will make it in 1.5, but it is not > must have show-stopper for hot-plug if it misses it. > OK, so without it we would only support a very strict subset of device_add command (specifically: only if the device_add arguments match exactly what was given to -cpu). This would work if carefully and clearly documented, true. But if we are going to have such a severe limitation, why not use "cpu_set" in 1.5 instead? cpu_set is not perfect, not the long-term solution we want, and it is also very limited, but at least it is already supported by libvirt and won't let users shoot their own foot.
On Tue, 26 Feb 2013 16:16:55 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 26, 2013 at 08:03:16PM +0100, Igor Mammedov wrote: > [...] > > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > > > > > > case, the common case would be to call "device_add XXX" with no > > > > > > > > > extra options at all, so there's no option to be excluded and no > > > > > > > > > special case to care about. > > > > > > > > That is if global properties will made to 1.5 which I highly doubt > > > > > > > > taking in account how fast patches are reviewed and accepted. > > > > > > > > Otherwise release would be broken. > > > > > > > > > > > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > > > > > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > > > > > > + device_add in an incompatible way. > > > > > > if all -cpu features are converted to static properties, we do not have to > > > > > > have global properties working. In absence of 'global properties', user > > > > > > will have to use the same properties at device_add that was specified at > > > > > > -cpu. And introduction of global properties won't regress it, it will > > > > > > just allow to use device_add without features specified on -cpu > > > > > > > > > > Strictly, we do not have to, but changing -cpu to set global properties > > > > > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed > > > > > by "device_add XXX" in an incompatible way. So if our long-term plan is > > > > Could you explain how ^^^ it will be incompatible, pls? > > > > > > Suppose that "foo" defaults to 0, and we run: "-cpu XXX,foo=1", followed > > > by "device_add XXX". > > > > > > Without globals/defaults set by -cpu, the above will create a new CPU > > > with foo=0. > > > > > > With globals/defaults set by -cpu, the above will create a new CPU with > > > foo=1. > > > > > > If I recall correctly, we agreed that the latter is the behavior we > > > wanted (because it is simpler for users, matches the fact that "-cpu" > > > already affects multiple CPU devices [it already affects all the CPUs > > > created on startup], and is the most common use-case [creating CPUs that > > > look basically the same]). > > Yes, that is the goal. I wouldn't say incompatible if user will start > > QEMU with "-cpu XXX,foo=1" and then use "device_add XXX,foo=1". That's a > > strict minimum that would work for hot-plug. Plain "device_add XXX" is an > > invalid in this case since it won't produce the same CPU. > > So later on top of "-cpu XXX,foo=1" + "device_add XXX,foo=1" we add up > > conversion of -cpu to global properties it shouldn't break anything, only add > > new option to create the same CPU usin "device_add XXX", users will still be > > able to use "device_add XXX,foo=1" if desired. > > > > I hope that -cpu => global properties will make it in 1.5, but it is not > > must have show-stopper for hot-plug if it misses it. > > > > OK, so without it we would only support a very strict subset of > device_add command (specifically: only if the device_add arguments match > exactly what was given to -cpu). > > This would work if carefully and clearly documented, true. But if we are > going to have such a severe limitation, why not use "cpu_set" in 1.5 > instead? cpu_set is not perfect, not the long-term solution we want, and > it is also very limited, but at least it is already supported by libvirt > and won't let users shoot their own foot. cpu_set is backup plan that I'm working on. So in case properties/subclasses won't make into 1.5, I'll post cpu_set alternative. > > -- > Eduardo >
On Tue, Feb 26, 2013 at 08:22:13PM +0100, Igor Mammedov wrote: > On Tue, 26 Feb 2013 16:16:55 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Feb 26, 2013 at 08:03:16PM +0100, Igor Mammedov wrote: > > [...] > > > > > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In this > > > > > > > > > > case, the common case would be to call "device_add XXX" with no > > > > > > > > > > extra options at all, so there's no option to be excluded and no > > > > > > > > > > special case to care about. > > > > > > > > > That is if global properties will made to 1.5 which I highly doubt > > > > > > > > > taking in account how fast patches are reviewed and accepted. > > > > > > > > > Otherwise release would be broken. > > > > > > > > > > > > > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add usable > > > > > > > > for CPU hotplug. Otherwise we would have to change the behavior of -cpu > > > > > > > > + device_add in an incompatible way. > > > > > > > if all -cpu features are converted to static properties, we do not have to > > > > > > > have global properties working. In absence of 'global properties', user > > > > > > > will have to use the same properties at device_add that was specified at > > > > > > > -cpu. And introduction of global properties won't regress it, it will > > > > > > > just allow to use device_add without features specified on -cpu > > > > > > > > > > > > Strictly, we do not have to, but changing -cpu to set global properties > > > > > > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed > > > > > > by "device_add XXX" in an incompatible way. So if our long-term plan is > > > > > Could you explain how ^^^ it will be incompatible, pls? > > > > > > > > Suppose that "foo" defaults to 0, and we run: "-cpu XXX,foo=1", followed > > > > by "device_add XXX". > > > > > > > > Without globals/defaults set by -cpu, the above will create a new CPU > > > > with foo=0. > > > > > > > > With globals/defaults set by -cpu, the above will create a new CPU with > > > > foo=1. > > > > > > > > If I recall correctly, we agreed that the latter is the behavior we > > > > wanted (because it is simpler for users, matches the fact that "-cpu" > > > > already affects multiple CPU devices [it already affects all the CPUs > > > > created on startup], and is the most common use-case [creating CPUs that > > > > look basically the same]). > > > Yes, that is the goal. I wouldn't say incompatible if user will start > > > QEMU with "-cpu XXX,foo=1" and then use "device_add XXX,foo=1". That's a > > > strict minimum that would work for hot-plug. Plain "device_add XXX" is an > > > invalid in this case since it won't produce the same CPU. > > > So later on top of "-cpu XXX,foo=1" + "device_add XXX,foo=1" we add up > > > conversion of -cpu to global properties it shouldn't break anything, only add > > > new option to create the same CPU usin "device_add XXX", users will still be > > > able to use "device_add XXX,foo=1" if desired. > > > > > > I hope that -cpu => global properties will make it in 1.5, but it is not > > > must have show-stopper for hot-plug if it misses it. > > > > > > > OK, so without it we would only support a very strict subset of > > device_add command (specifically: only if the device_add arguments match > > exactly what was given to -cpu). > > > > This would work if carefully and clearly documented, true. But if we are > > going to have such a severe limitation, why not use "cpu_set" in 1.5 > > instead? cpu_set is not perfect, not the long-term solution we want, and > > it is also very limited, but at least it is already supported by libvirt > > and won't let users shoot their own foot. > cpu_set is backup plan that I'm working on. > So in case properties/subclasses won't make into 1.5, I'll post cpu_set > alternative. OK. But I would also suggest that we follow the cpu_set plan in case we don't manage to make "-cpu" set defaults/global-properties in 1.5.
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs index c1d4f05..887dca7 100644 --- a/target-i386/Makefile.objs +++ b/target-i386/Makefile.objs @@ -2,7 +2,7 @@ obj-y += translate.o helper.o cpu.o obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o arch_dump.o -obj-$(CONFIG_KVM) += kvm.o hyperv.o +obj-$(CONFIG_KVM) += kvm.o obj-$(CONFIG_NO_KVM) += kvm-stub.o obj-$(CONFIG_LINUX_USER) += ioport-user.o obj-$(CONFIG_BSD_USER) += ioport-user.o diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 5626931..35fc303 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -33,8 +33,6 @@ #include "qapi/visitor.h" #include "sysemu/arch_init.h" -#include "hyperv.h" - #include "hw/hw.h" #if defined(CONFIG_KVM) #include <linux/kvm_para.h> @@ -1422,12 +1420,19 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp); } else if (!strcmp(featurestr, "hv-spinlocks")) { char *err; + const int min = 0xFFF; numvalue = strtoul(val, &err, 0); if (!*val || *err) { error_setg(errp, "bad numerical value %s", val); goto out; } - hyperv_set_spinlock_retries(numvalue); + if (numvalue < min) { + fprintf(stderr, "hv-spinlocks value shall always be >= 0x%x" + ", fixup will be removed in future versions\n", + min); + numvalue = min; + } + env->hyperv_spinlock_attempts = numvalue; } else { error_setg(errp, "unrecognized feature %s", featurestr); goto out; @@ -1437,9 +1442,9 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) } else if (!strcmp(featurestr, "enforce")) { check_cpuid = enforce_cpuid = 1; } else if (!strcmp(featurestr, "hv_relaxed")) { - hyperv_enable_relaxed_timing(true); + env->hyperv_relaxed_timing = true; } else if (!strcmp(featurestr, "hv_vapic")) { - hyperv_enable_vapic_recommended(true); + env->hyperv_vapic = true; } else { error_setg(errp, "feature string `%s' not in format (+feature|" "-feature|feature=xyz)", featurestr); @@ -1592,6 +1597,7 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp) def->kvm_features |= kvm_default_features; } def->ext_features |= CPUID_EXT_HYPERVISOR; + env->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp); object_property_set_int(OBJECT(cpu), def->level, "level", errp); diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 7577e4f..e3723c2 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -549,6 +549,10 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_MWAIT_IBE (1 << 1) /* Interrupts can exit capability */ #define CPUID_MWAIT_EMX (1 << 0) /* enumeration supported */ +#ifndef HYPERV_SPINLOCK_NEVER_RETRY +#define HYPERV_SPINLOCK_NEVER_RETRY 0xFFFFFFFF +#endif + #define EXCP00_DIVZ 0 #define EXCP01_DB 1 #define EXCP02_NMI 2 @@ -840,6 +844,9 @@ typedef struct CPUX86State { uint32_t cpuid_ext4_features; /* Flags from CPUID[EAX=7,ECX=0].EBX */ uint32_t cpuid_7_0_ebx_features; + bool hyperv_vapic; + bool hyperv_relaxed_timing; + int hyperv_spinlock_attempts; /* MTRRs */ uint64_t mtrr_fixed[11]; diff --git a/target-i386/hyperv.c b/target-i386/hyperv.c deleted file mode 100644 index f284e99..0000000 --- a/target-i386/hyperv.c +++ /dev/null @@ -1,64 +0,0 @@ -/* - * QEMU Hyper-V support - * - * Copyright Red Hat, Inc. 2011 - * - * Author: Vadim Rozenfeld <vrozenfe@redhat.com> - * - * This work is licensed under the terms of the GNU GPL, version 2 or later. - * See the COPYING file in the top-level directory. - * - */ - -#include "hyperv.h" - -static bool hyperv_vapic; -static bool hyperv_relaxed_timing; -static int hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; - -void hyperv_enable_vapic_recommended(bool val) -{ - hyperv_vapic = val; -} - -void hyperv_enable_relaxed_timing(bool val) -{ - hyperv_relaxed_timing = val; -} - -void hyperv_set_spinlock_retries(int val) -{ - hyperv_spinlock_attempts = val; - if (hyperv_spinlock_attempts < 0xFFF) { - hyperv_spinlock_attempts = 0xFFF; - } -} - -bool hyperv_enabled(void) -{ - return hyperv_hypercall_available() || hyperv_relaxed_timing_enabled(); -} - -bool hyperv_hypercall_available(void) -{ - if (hyperv_vapic || - (hyperv_spinlock_attempts != HYPERV_SPINLOCK_NEVER_RETRY)) { - return true; - } - return false; -} - -bool hyperv_vapic_recommended(void) -{ - return hyperv_vapic; -} - -bool hyperv_relaxed_timing_enabled(void) -{ - return hyperv_relaxed_timing; -} - -int hyperv_get_spinlock_retries(void) -{ - return hyperv_spinlock_attempts; -} diff --git a/target-i386/hyperv.h b/target-i386/hyperv.h deleted file mode 100644 index bacb1d4..0000000 --- a/target-i386/hyperv.h +++ /dev/null @@ -1,45 +0,0 @@ -/* - * QEMU Hyper-V support - * - * Copyright Red Hat, Inc. 2011 - * - * Author: Vadim Rozenfeld <vrozenfe@redhat.com> - * - * This work is licensed under the terms of the GNU GPL, version 2 or later. - * See the COPYING file in the top-level directory. - * - */ - -#ifndef QEMU_HW_HYPERV_H -#define QEMU_HW_HYPERV_H 1 - -#include "qemu-common.h" -#ifdef CONFIG_KVM -#include <asm/hyperv.h> -#endif - -#ifndef HYPERV_SPINLOCK_NEVER_RETRY -#define HYPERV_SPINLOCK_NEVER_RETRY 0xFFFFFFFF -#endif - -#ifndef KVM_CPUID_SIGNATURE_NEXT -#define KVM_CPUID_SIGNATURE_NEXT 0x40000100 -#endif - -#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_KVM) -void hyperv_enable_vapic_recommended(bool val); -void hyperv_enable_relaxed_timing(bool val); -void hyperv_set_spinlock_retries(int val); -#else -static inline void hyperv_enable_vapic_recommended(bool val) { } -static inline void hyperv_enable_relaxed_timing(bool val) { } -static inline void hyperv_set_spinlock_retries(int val) { } -#endif - -bool hyperv_enabled(void); -bool hyperv_hypercall_available(void); -bool hyperv_vapic_recommended(void); -bool hyperv_relaxed_timing_enabled(void); -int hyperv_get_spinlock_retries(void); - -#endif /* QEMU_HW_HYPERV_H */ diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 0cf413d..9889388 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -31,7 +31,7 @@ #include "hw/pc.h" #include "hw/apic.h" #include "exec/ioport.h" -#include "hyperv.h" +#include <asm/hyperv.h> #include "hw/pci/pci.h" //#define DEBUG_KVM @@ -417,6 +417,22 @@ unsigned long kvm_arch_vcpu_id(CPUState *cs) return cpu->env.cpuid_apic_id; } +#ifndef KVM_CPUID_SIGNATURE_NEXT +#define KVM_CPUID_SIGNATURE_NEXT 0x40000100 +#endif + +static bool hyperv_hypercall_available(CPUX86State *env) +{ + return env->hyperv_vapic || + (env->hyperv_spinlock_attempts != HYPERV_SPINLOCK_NEVER_RETRY); +} + +static bool hyperv_enabled(CPUX86State *env) +{ + return hyperv_hypercall_available(env) || + env->hyperv_relaxed_timing; +} + #define KVM_MAX_CPUID_ENTRIES 100 int kvm_arch_init_vcpu(CPUState *cs) @@ -439,7 +455,7 @@ int kvm_arch_init_vcpu(CPUState *cs) c = &cpuid_data.entries[cpuid_i++]; memset(c, 0, sizeof(*c)); c->function = KVM_CPUID_SIGNATURE; - if (!hyperv_enabled()) { + if (!hyperv_enabled(env)) { memcpy(signature, "KVMKVMKVM\0\0\0", 12); c->eax = 0; } else { @@ -455,7 +471,7 @@ int kvm_arch_init_vcpu(CPUState *cs) c->function = KVM_CPUID_FEATURES; c->eax = env->cpuid_kvm_features; - if (hyperv_enabled()) { + if (hyperv_enabled(env)) { memcpy(signature, "Hv#1\0\0\0\0\0\0\0\0", 12); c->eax = signature[0]; @@ -468,10 +484,10 @@ int kvm_arch_init_vcpu(CPUState *cs) c = &cpuid_data.entries[cpuid_i++]; memset(c, 0, sizeof(*c)); c->function = HYPERV_CPUID_FEATURES; - if (hyperv_relaxed_timing_enabled()) { + if (env->hyperv_relaxed_timing) { c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; } - if (hyperv_vapic_recommended()) { + if (env->hyperv_vapic) { c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; c->eax |= HV_X64_MSR_APIC_ACCESS_AVAILABLE; } @@ -479,13 +495,13 @@ int kvm_arch_init_vcpu(CPUState *cs) c = &cpuid_data.entries[cpuid_i++]; memset(c, 0, sizeof(*c)); c->function = HYPERV_CPUID_ENLIGHTMENT_INFO; - if (hyperv_relaxed_timing_enabled()) { + if (env->hyperv_relaxed_timing) { c->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED; } - if (hyperv_vapic_recommended()) { + if (env->hyperv_vapic) { c->eax |= HV_X64_APIC_ACCESS_RECOMMENDED; } - c->ebx = hyperv_get_spinlock_retries(); + c->ebx = env->hyperv_spinlock_attempts; c = &cpuid_data.entries[cpuid_i++]; memset(c, 0, sizeof(*c)); @@ -1107,11 +1123,11 @@ static int kvm_put_msrs(X86CPU *cpu, int level) kvm_msr_entry_set(&msrs[n++], MSR_KVM_PV_EOI_EN, env->pv_eoi_en_msr); } - if (hyperv_hypercall_available()) { + if (hyperv_hypercall_available(env)) { kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_GUEST_OS_ID, 0); kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_HYPERCALL, 0); } - if (hyperv_vapic_recommended()) { + if (env->hyperv_vapic) { kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 0); } }
- since hyperv_* helper functions are used only in target-i386/kvm.c move them there as static helpers Signed-off-by: Igor Mammedov <imammedo@redhat.com> Requestd-By: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/Makefile.objs | 2 +- target-i386/cpu.c | 16 +++++++--- target-i386/cpu.h | 7 +++++ target-i386/hyperv.c | 64 --------------------------------------------- target-i386/hyperv.h | 45 ------------------------------- target-i386/kvm.c | 36 ++++++++++++++++++------- 6 files changed, 45 insertions(+), 125 deletions(-) delete mode 100644 target-i386/hyperv.c delete mode 100644 target-i386/hyperv.h