Re: [PATCH v2/RFC] libkvm-s390

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Christian Borntraeger wrote:
> This is an update patch for libkvm to build and work on s390
[...]
> Index: kvm-userspace/libkvm/libkvm-s390.c
> ===================================================================
> --- /dev/null
> +++ kvm-userspace/libkvm/libkvm-s390.c
> @@ -0,0 +1,137 @@
> +/*
> + * This file contains the s390 specific implementation for the
> + * architecture dependent functions defined in kvm-common.h and
> + * libkvm.h
> + *
> + * Copyright (C) 2006 Qumranet
> + * Copyright IBM Corp. 2008
> + *
> + * Authors:
> + *	Carsten Otte <cotte@de.ibm.com>
> + *      Christian Borntraeger <borntraeger@de.ibm.com>
>   
whitespace error - well just in the comment, but if you resubmit it 
anyway this space could be removed ;-)

[...]
> +
> +int handle_dcr(struct kvm_run *run,  kvm_context_t kvm, int vcpu)
> +{
> +	fprintf(stderr, "%s: Operation not supported\n", __FUNCTION__);
> +	return -1;
> +}
> +
>   

I think Carsten started with the powerpc file here.
This function is powerpc only and even in our code only called later on 
in this libkvm-$arch.c file.
Maybe we should patch our file and make it static to clarify that.
Since you don't have that call its superfluous, you should be able to 
drop it without consequence.

[...]
> Index: kvm-userspace/libkvm/libkvm.c
> ===================================================================
> --- kvm-userspace.orig/libkvm/libkvm.c
> +++ kvm-userspace/libkvm/libkvm.c
> @@ -48,6 +48,10 @@
>  #include "kvm-powerpc.h"
>  #endif
>
> +#if defined(__s390__)
> +#include "kvm-s390.h"
> +#endif
> +
>  int kvm_abi = EXPECTED_KVM_API_VERSION;
>  int kvm_page_size;
>
> @@ -88,7 +92,11 @@ int get_free_slot(kvm_context_t kvm)
>  	if (tss_ext > 0)
>  		i = 0;
>  	else
> +#if !defined(__s390__)
>  		i = 1;
> +#else
> +		i = 0;
> +#endif
>
>   
This looks a bit complicated, but when thinking of the result this just 
means that i is always 0 on s390.
Therefore I think it would look less confusing when the ifdef would not 
be in the else part of that tss_ext if.

Since i will always be zero in s390 case you could do things like
- initialize i with 0 and let the whole block until the for loop run in 
"if !defined(__s390__)"

That way you would additionally stop modifying the tss_ext variable 
which is not used in the s390 case anyway.

[...]

-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization