Re: [PATCH] kvm tools: Clean up LINT assignment code

[Sasha Levin <levinsasha928@gmail.com>]

On Wed, 2011-12-14 at 13:06 +1100, Matt Evans wrote:
> Hi Sasha,
> 
> On 12/12/11 06:50, Sasha Levin wrote:
> > Just set delivery mode directly without going through ugly casting.
> > 
> > This cleans up and simplifies the code.
> > 
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> >  tools/kvm/x86/kvm-cpu.c |   10 ++--------
> >  1 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/kvm/x86/kvm-cpu.c b/tools/kvm/x86/kvm-cpu.c
> > index 27b7a8f..cc1f560 100644
> > --- a/tools/kvm/x86/kvm-cpu.c
> > +++ b/tools/kvm/x86/kvm-cpu.c
> > @@ -81,18 +81,12 @@ static int kvm_cpu__set_lint(struct kvm_cpu *vcpu)
> >  {
> >  	struct kvm_lapic_state klapic;
> >  	struct local_apic *lapic = (void *)&klapic;
> > -	u32 lvt;
> >  
> >  	if (ioctl(vcpu->vcpu_fd, KVM_GET_LAPIC, &klapic))
> >  		return -1;
> >  
> > -	lvt = *(u32 *)&lapic->lvt_lint0;
> > -	lvt = SET_APIC_DELIVERY_MODE(lvt, APIC_MODE_EXTINT);
> > -	*(u32 *)&lapic->lvt_lint0 = lvt;
> > -
> > -	lvt = *(u32 *)&lapic->lvt_lint1;
> > -	lvt = SET_APIC_DELIVERY_MODE(lvt, APIC_MODE_NMI);
> > -	*(u32 *)&lapic->lvt_lint1 = lvt;
> > +	lapic->lvt_lint0.delivery_mode = APIC_MODE_EXTINT;
> > +	lapic->lvt_lint1.delivery_mode = APIC_MODE_NMI;
> >  
> >  	return ioctl(vcpu->vcpu_fd, KVM_SET_LAPIC, &klapic);
> >  }
> 
> I'm getting this on x86-32, gcc 4.4.3:
> 
>   CC       x86/kvm-cpu.o
> cc1: warnings being treated as errors
> x86/kvm-cpu.c: In function ‘kvm_cpu__set_lint’:
> x86/kvm-cpu.c:89: error: dereferencing pointer ‘lapic’ does break strict-aliasing rules
> x86/kvm-cpu.c:88: error: dereferencing pointer ‘lapic’ does break strict-aliasing rules
> x86/kvm-cpu.c:83: note: initialized from here
> make: *** [x86/kvm-cpu.o] Error 1
> 
> Removing the nasty aliasing (patch below) seems to be a good way to go.  What do
> you think?
> 
> 
> Cheers,
> 
> 
> Matt
> 
> ---
> 
> diff --git a/tools/kvm/x86/kvm-cpu.c b/tools/kvm/x86/kvm-cpu.c
> index cc1f560..30f1ad6 100644
> --- a/tools/kvm/x86/kvm-cpu.c
> +++ b/tools/kvm/x86/kvm-cpu.c
> @@ -79,16 +79,15 @@ void kvm_cpu__delete(struct kvm_cpu *vcpu)
>  
>  static int kvm_cpu__set_lint(struct kvm_cpu *vcpu)
>  {
> -	struct kvm_lapic_state klapic;
> -	struct local_apic *lapic = (void *)&klapic;
> +	struct local_apic lapic;
>  
> -	if (ioctl(vcpu->vcpu_fd, KVM_GET_LAPIC, &klapic))
> +	if (ioctl(vcpu->vcpu_fd, KVM_GET_LAPIC, &lapic))
>  		return -1;

It felt a bit wrong to me getting the ioctl to assign it directly to
local_apic since api.txt says it should be returning a kvm_lapic_state.
But on the other hand, api.txt also says "The data format and layout are
the same as documented in the architecture manual." so I guess we can go
with that.

	Acked-by: Sasha Levin <levinsasha928@gmail.com>

-- 

Sasha.