On Thu, Jul 26, 2018 at 09:29:44AM +0200, David Hildenbrand wrote: > On 25.07.2018 22:14, Eduardo Habkost wrote: > > On Wed, Jul 25, 2018 at 07:50:21PM +0200, David Hildenbrand wrote: > >> On 25.07.2018 19:09, Eduardo Habkost wrote: > > [...] > >>>> + if (local_err) { > >>>> + g_assert(kvm_enabled()); > >>>> + error_report_err(local_err); > >>>> + /* fallback to unsupported CPU models */ > >>>> + return; > >>> > >>> What about moving this outside instance_init? > >> > >> To which place for example? We at least have to copy the CPU model > >> for each and every CPU instance (so we can modify it without side > >> effects using properties). > > > > To any code that will look at cpu->model. > > > > You are wrapping an interface that needs to report errors > > (kvm_s390_get_host_cpu_model()) behind an interface that is not > > able to report errors (object_new()). There's nothing that > > requires you to do that, does it? You are free to provide an API > > that is actually able to report errors, instead of relying on > > object_new() only. > > I see what you mean. One solution would be to preload and store the > model somewhere globally (not locally). So in the init function, we > would not have to handle errors. > > But I am not even sure where we could do such a global initialization + > be able to report errors easily. I remember that we had a hard time to > get this running smoothly due to the dependency of > kvm_s390_get_host_cpu_model() on: > - accelerator > - machine > - KVM init state
If we had a S390KVMAccelerator object on machine->accelerator, S390KVMAccelerator::host_model would be a good candidate? > > And initializing cpu->model in realize() is too late, because all > properties have to access it. Even a pre_plug handler will not work. Yeah, the instance_init/realize abstraction seems insufficient here. instance_init has too many restrictions, realize is too late. > > On the other hand, I decided to ignore all errors back then and fallback > to the "host CPU model unknown" case, because there are some corner > cases where we still want to allow running the "host" model even though > there was a problem detecting it. > > So my summary would be: We ignore errors (and rather treat them like > warnings) for a reason here and fallback to "unsupported CPU models", > which allows to run + use QEMU even in environments where our CPU model > detection fails (e.g. on a very strange new CPU model we could have in > the future). > > Especially "!cpu->model" does not imply that there was an error. It > includes disabled CPU model support or unavailable CPU model support > (KVM), which is perfectly fine. Replicating initialization attempts at > all places where we access "cpu->model" does therefore not sound 100% > clean to me and most likely makes the code way more complicated. > > Right now the semantics are clear: if we have "!cpu->model" after the > object has been created, details about the host CPU model are not > available (models unavailable/unsupported). Modifying properties, > baselining, expanding is not possible with that model then. But it can > be used for execution. This is interesting. If most users of cpu->model don't care about kvm_s390_get_host_cpu_model() errors at all, the current solution sounds more reasonable. Except for the error_report_err() call inside instance_init. This still bothers me, but it's not a big deal. -- Eduardo