On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote:
[...]
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 4d75ff0..94fede5 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -276,12 +276,59 @@ static int cpu_model_set(KVMState *s, uint64_t attr,
> void *addr)
> return rc;
> }
>
> -static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
This seems to duplicate lots of the existing KVM code. (See additional
comment below about possible ways to avoid it).
> +static int get_machine_props_fallback(S390MachineProps *prop)
> +{
> + struct kvm_device_attr dev_attr;
> + int rc, kvmfd = -1, vmfd = -1;
> +
> + rc = qemu_open("/dev/kvm", O_RDWR);
> + if (rc < 0) {
> + goto out_err;
> + }
> + kvmfd = rc;
> +
> + rc = ioctl(kvmfd, KVM_CREATE_VM, 0);
> + if (rc < 0) {
> + goto out_err;
> + }
> + vmfd = rc;
> +
> + rc = ioctl(vmfd, KVM_CHECK_EXTENSION, KVM_CAP_VM_ATTRIBUTES);
> + if (rc < 0) {
> + rc = -ENOSYS;
> + goto out_err;
> + }
> +
> + dev_attr.group = KVM_S390_VM_CPU_MODEL;
> + dev_attr.attr = KVM_S390_VM_CPU_MACHINE;
> + rc = ioctl(vmfd, KVM_HAS_DEVICE_ATTR, &dev_attr);
> + if (rc < 0) {
> + rc = -EFAULT;
> + goto out_err;
> + }
> +
> + dev_attr.addr = (uint64_t) prop;
> + rc = ioctl(vmfd, KVM_GET_DEVICE_ATTR, &dev_attr);
> +
> +out_err:
> + if (vmfd >= 0) {
> + close(vmfd);
> + }
> + if (kvmfd >= 0) {
> + close(kvmfd);
> + }
> +
> + return rc;
> +}
> +
> +int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> {
> int rc = -EFAULT;
>
> if (s) {
> rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, prop);
> + } else {
> + rc = get_machine_props_fallback(prop);
> }
The comments below are just suggestions, not something which should
block the patch, in my opinion:
First, if s is always NULL inside arch_query_cpu_definitions(), and is
always non-NULL inside kvm_setup_cpu_classes(), why don't you just call
keep the original kvm_s390_get_machine_props() function, and call and
get_machine_props_fallback() inside arch_query_cpu_definitions()?
The only thing common to both cases is the tracing point, but if we are
running two completely different code paths I assume it would be a good
thing to have a different tracing point for
get_machine_props_fallback().
Second, you shouldn't even need to duplicate code in
get_machine_props_fallback() if you are able to create an accel object
and do just basic initialization so that cpu_model_get() works.
Allowing accel objects to be created on the fly was one of the main
purposes of the accel QOM work.
For example, if we do something like this:
https://github.com/ehabkost/qemu-hacks/commit/36a250e34c5fd0d43a25271f5bc9b04681fdd56a
[1]
https://github.com/ehabkost/qemu-hacks/commits/work/accel-open-func
Then the code could look like this:
accel.c:
/* configure_accelerator() would be changed to reuse this function: */
AccelState *accel_create(const char *accel_name)
{
AccelClass *acc = accel_find(accel_name);
/*TODO: error handling, checking acc->available() */
return ACCEL(object_new(object_class_get_name(OBJECT_CLASS(acc))));
}
/* Do basic accel initialization without affecting global QEMU state */
/* accel_init_machine() would be changed to reuse this function: */
void accel_open(AccelState *s, Error **errp)
{
object_property_set_bool(OBJECT(s), true, "open", errp);
}
target-s390/kvm.c:
/* Using a different function name would be interesting, as it would be
* the main arch_query_cpu_definitions() code path, not a fallback.
*/
int get_machine_props_fallback(S390MachineProps *prop)
{
int r;
AccelState *ac = accel_create("kvm");
/*TODO: error handling */
accel_open(ac, &err);
r = cpu_model_get(ac, prop);
object_unref(OBJECT(ac));
return r;
}
[1] I only moved the /dev/kvm opening to the open method, but maybe the
whole code up to KVM_CREATE_VM and capabitlity checking could be
moved.
(But I don't know how to handle kvm_type, as it is currently
provided by MachineClass. Maybe kvm_type() belongs to CPUClass
instead of MachineClass?)
> trace_kvm_get_machine_props(rc, prop->cpuid, prop->ibc);
>
> --
> 1.8.3.1
>
--
Eduardo
