[PATCH] always report x2apic as supported feature

[PATCH] always report x2apic as supported feature

[Gleb Natapov]

We emulate x2apic in software, so host support is not required.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00844eb..c256da7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1497,6 +1497,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 1:
 		entry->edx &= kvm_supported_word0_x86_features;
 		entry->ecx &= kvm_supported_word4_x86_features;
+		/* we support x2apic emulation even if host does not support
+		   it since we emulate x2apic in software */
+		entry->ecx |= F(X2APIC);
 		break;
 	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
 	 * may return different values. This forces us to get_cpu() before
--
			Gleb.

Re: [PATCH] always report x2apic as supported feature

[Avi Kivity]

On 07/12/2009 04:10 PM, Gleb Natapov wrote:
> We emulate x2apic in software, so host support is not required.
>
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] always report x2apic as supported feature

[Marcelo Tosatti]

On Sun, Jul 12, 2009 at 04:10:55PM +0300, Gleb Natapov wrote:
> We emulate x2apic in software, so host support is not required.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 00844eb..c256da7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1497,6 +1497,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	case 1:
>  		entry->edx &= kvm_supported_word0_x86_features;
>  		entry->ecx &= kvm_supported_word4_x86_features;
> +		/* we support x2apic emulation even if host does not support
> +		   it since we emulate x2apic in software */
> +		entry->ecx |= F(X2APIC);
>  		break;
>  	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
>  	 * may return different values. This forces us to get_cpu() before
> --
> 			Gleb.

What if you have an older host that does not support emulate x2apic?

Re: [PATCH] always report x2apic as supported feature

[Sheng Yang]

On Thursday 16 July 2009 07:01:30 Marcelo Tosatti wrote:
> On Sun, Jul 12, 2009 at 04:10:55PM +0300, Gleb Natapov wrote:
> > We emulate x2apic in software, so host support is not required.
> >
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 00844eb..c256da7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1497,6 +1497,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2
> > *entry, u32 function, case 1:
> >  		entry->edx &= kvm_supported_word0_x86_features;
> >  		entry->ecx &= kvm_supported_word4_x86_features;
> > +		/* we support x2apic emulation even if host does not support
> > +		   it since we emulate x2apic in software */
> > +		entry->ecx |= F(X2APIC);
> >  		break;
> >  	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
> >  	 * may return different values. This forces us to get_cpu() before
> > --
> > 			Gleb.
>
> What if you have an older host that does not support emulate x2apic?

Due to interrupt remapping can't be enabled with KVM now, I think older host 
would just ignore this info... (The new one can work without interrupt 
remapping with KVM).

By the way, I saw X2APIC in host supported CPUID feature list(1.ecx), which I 
don't think it's very properly. Host x2apic feature have nothing to do with 
KVM, we do the emulation all the way. I suggest to remove the mask for host, 
and give a comment that we would emulate all x2apic behaviour here, rather 
than "even if", which I think it's a little misleading.

-- 
regards
Yang, Sheng

Re: [PATCH] always report x2apic as supported feature

[Gleb Natapov]

On Thu, Jul 16, 2009 at 09:46:21AM +0800, Sheng Yang wrote:
> On Thursday 16 July 2009 07:01:30 Marcelo Tosatti wrote:
> > On Sun, Jul 12, 2009 at 04:10:55PM +0300, Gleb Natapov wrote:
> > > We emulate x2apic in software, so host support is not required.
> > >
> > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 00844eb..c256da7 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -1497,6 +1497,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2
> > > *entry, u32 function, case 1:
> > >  		entry->edx &= kvm_supported_word0_x86_features;
> > >  		entry->ecx &= kvm_supported_word4_x86_features;
> > > +		/* we support x2apic emulation even if host does not support
> > > +		   it since we emulate x2apic in software */
> > > +		entry->ecx |= F(X2APIC);
> > >  		break;
> > >  	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
> > >  	 * may return different values. This forces us to get_cpu() before
> > > --
> > > 			Gleb.
> >
> > What if you have an older host that does not support emulate x2apic?
> 
> Due to interrupt remapping can't be enabled with KVM now, I think older host 
> would just ignore this info... (The new one can work without interrupt 
> remapping with KVM).
> 
> By the way, I saw X2APIC in host supported CPUID feature list(1.ecx), which I 
Where have you seen it? If you mean kvm_supported_word4_x86_features
then it is not what is supported by the host, but what is supported by
KVM. Host unsupported bits are dropped from there before reporting to
userspace. That is why this patch what necessary.

> don't think it's very properly. Host x2apic feature have nothing to do with 
> KVM, we do the emulation all the way. I suggest to remove the mask for host, 
> and give a comment that we would emulate all x2apic behaviour here, rather 
> than "even if", which I think it's a little misleading.
> 
> -- 
> regards
> Yang, Sheng

--
			Gleb.

Re: [PATCH] always report x2apic as supported feature

[Sheng Yang]

On Thursday 16 July 2009 14:00:15 Gleb Natapov wrote:
> On Thu, Jul 16, 2009 at 09:46:21AM +0800, Sheng Yang wrote:
> > On Thursday 16 July 2009 07:01:30 Marcelo Tosatti wrote:
> > > On Sun, Jul 12, 2009 at 04:10:55PM +0300, Gleb Natapov wrote:
> > > > We emulate x2apic in software, so host support is not required.
> > > >
> > > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index 00844eb..c256da7 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -1497,6 +1497,9 @@ static void do_cpuid_ent(struct
> > > > kvm_cpuid_entry2 *entry, u32 function, case 1:
> > > >  		entry->edx &= kvm_supported_word0_x86_features;
> > > >  		entry->ecx &= kvm_supported_word4_x86_features;
> > > > +		/* we support x2apic emulation even if host does not support
> > > > +		   it since we emulate x2apic in software */
> > > > +		entry->ecx |= F(X2APIC);
> > > >  		break;
> > > >  	/* function 2 entries are STATEFUL. That is, repeated cpuid
> > > > commands * may return different values. This forces us to get_cpu()
> > > > before --
> > > > 			Gleb.
> > >
> > > What if you have an older host that does not support emulate x2apic?
> >
> > Due to interrupt remapping can't be enabled with KVM now, I think older
> > host would just ignore this info... (The new one can work without
> > interrupt remapping with KVM).
> >
> > By the way, I saw X2APIC in host supported CPUID feature list(1.ecx),
> > which I
>
> Where have you seen it? If you mean kvm_supported_word4_x86_features
> then it is not what is supported by the host, but what is supported by
> KVM. Host unsupported bits are dropped from there before reporting to
> userspace. That is why this patch what necessary.

Yes, that's what I mean. x2apic feature needn't judged by host feature, we can 
always set the bit to support it, no need for a filter. I think put it in the 
kvm_supported_word4_x86_features is a little misleading means that KVM support 
it through host feature rather than emulation.

Anyway, not a big deal.

-- 
regards
Yang, Sheng

>
> > don't think it's very properly. Host x2apic feature have nothing to do
> > with KVM, we do the emulation all the way. I suggest to remove the mask
> > for host, and give a comment that we would emulate all x2apic behaviour
> > here, rather than "even if", which I think it's a little misleading.
> >
> > --
> > regards
> > Yang, Sheng
>
> --
> 			Gleb.

Re: [PATCH] always report x2apic as supported feature

[Gleb Natapov]

On Thu, Jul 16, 2009 at 02:09:09PM +0800, Sheng Yang wrote:
> On Thursday 16 July 2009 14:00:15 Gleb Natapov wrote:
> > On Thu, Jul 16, 2009 at 09:46:21AM +0800, Sheng Yang wrote:
> > > On Thursday 16 July 2009 07:01:30 Marcelo Tosatti wrote:
> > > > On Sun, Jul 12, 2009 at 04:10:55PM +0300, Gleb Natapov wrote:
> > > > > We emulate x2apic in software, so host support is not required.
> > > > >
> > > > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > index 00844eb..c256da7 100644
> > > > > --- a/arch/x86/kvm/x86.c
> > > > > +++ b/arch/x86/kvm/x86.c
> > > > > @@ -1497,6 +1497,9 @@ static void do_cpuid_ent(struct
> > > > > kvm_cpuid_entry2 *entry, u32 function, case 1:
> > > > >  		entry->edx &= kvm_supported_word0_x86_features;
> > > > >  		entry->ecx &= kvm_supported_word4_x86_features;
> > > > > +		/* we support x2apic emulation even if host does not support
> > > > > +		   it since we emulate x2apic in software */
> > > > > +		entry->ecx |= F(X2APIC);
> > > > >  		break;
> > > > >  	/* function 2 entries are STATEFUL. That is, repeated cpuid
> > > > > commands * may return different values. This forces us to get_cpu()
> > > > > before --
> > > > > 			Gleb.
> > > >
> > > > What if you have an older host that does not support emulate x2apic?
> > >
> > > Due to interrupt remapping can't be enabled with KVM now, I think older
> > > host would just ignore this info... (The new one can work without
> > > interrupt remapping with KVM).
> > >
> > > By the way, I saw X2APIC in host supported CPUID feature list(1.ecx),
> > > which I
> >
> > Where have you seen it? If you mean kvm_supported_word4_x86_features
> > then it is not what is supported by the host, but what is supported by
> > KVM. Host unsupported bits are dropped from there before reporting to
> > userspace. That is why this patch what necessary.
> 
> Yes, that's what I mean. x2apic feature needn't judged by host feature, we can 
> always set the bit to support it, no need for a filter. I think put it in the 
> kvm_supported_word4_x86_features is a little misleading means that KVM support 
> it through host feature rather than emulation.
> 
Yeah, I put it there initially since I misunderstood how things work and
thought that it will be reported to userspace (and usercpace had a bug
that prevented me from discovering the problem).

--
			Gleb.