* Re: [PATCHv2] x86info: dump kvm cpuid's
From: Michael S. Tsirkin @ 2012-05-01 12:35 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, kvm, pv-drivers, virtualization, devel, davej
In-Reply-To: <1335875225.6038.98.camel@zakaz.uk.xensource.com>
On Tue, May 01, 2012 at 01:27:05PM +0100, Ian Campbell wrote:
> On Mon, 2012-04-30 at 17:38 +0300, Michael S. Tsirkin wrote:
> > The following makes 'x86info -r' dump hypervisor leaf cpu ids
> > (for kvm this is signature+features) when running in a vm.
> >
> > On the guest we see the signature and the features:
> > eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> > eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
> >
> > Hypervisor flag is checked to avoid output changes when not
> > running on a VM.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Changes from v1:
> > Make work on non KVM hypervisors (only KVM was tested).
> > Avi Kivity said kvm will in the future report
> > max HV leaf in eax. For now it reports eax = 0
> > so add a work around for that.
> >
> > ---
> >
> > diff --git a/identify.c b/identify.c
> > index 33f35de..a4a3763 100644
> > --- a/identify.c
> > +++ b/identify.c
> > @@ -9,8 +9,8 @@
> >
> > void get_cpu_info_basics(struct cpudata *cpu)
> > {
> > - unsigned int maxi, maxei, vendor, address_bits;
> > - unsigned int eax;
> > + unsigned int maxi, maxei, maxhv, vendor, address_bits;
> > + unsigned int eax, ebx, ecx;
> >
> > cpuid(cpu->number, 0, &maxi, &vendor, NULL, NULL);
> > maxi &= 0xffff; /* The high-order word is non-zero on some Cyrix CPUs */
> > @@ -19,7 +19,7 @@ void get_cpu_info_basics(struct cpudata *cpu)
> > return;
> >
> > /* Everything that supports cpuid supports these. */
> > - cpuid(cpu->number, 1, &eax, NULL, NULL, NULL);
> > + cpuid(cpu->number, 1, &eax, &ebx, &ecx, NULL);
>
> You probably want to check ebx, ecx, edx for the signatures of the
> hypervisor's you are willing to support and which you know do something
> sane with eax?
Everyone puts the max leaf in eax - this is what hardware
CPUs do. So I think we can just interpret eax as such.
If someone wants to put broken info in cpuid,
send a patch to blacklist it.
> Also it would be something worth reporting in its own
> right?
>
> BTW, according to arch/x86/include/asm/kvm_para.h unsurprisingly KVM has
> a signature too 'KVMKVMKVM'.
>
> > cpu->stepping = eax & 0xf;
> > cpu->model = (eax >> 4) & 0xf;
> > cpu->family = (eax >> 8) & 0xf;
> > @@ -29,6 +29,19 @@ void get_cpu_info_basics(struct cpudata *cpu)
> >
> > cpuid(cpu->number, 0xC0000000, &maxei, NULL, NULL, NULL);
> > cpu->maxei2 = maxei;
> > + if (ecx & 0x80000000) {
> > + cpuid(cpu->number, 0x40000000, &maxhv, NULL, NULL, NULL);
> > + /*
> > + * KVM up to linux 3.4 reports 0 as the max hypervisor leaf,
> > + * where it really means 0x40000001.
>
> This is something where I definitely think you want to check the
> signature first.
>
> Ian.
kvm lets users change the signature.
But worst case we print one useless line.
Better than not printing something potentially useful.
--
MST
^ permalink raw reply
* Re: [PATCHv2] x86info: dump kvm cpuid's
From: Ian Campbell @ 2012-05-01 12:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, kvm, pv-drivers, virtualization, devel, davej
In-Reply-To: <20120430143835.GA10190@redhat.com>
On Mon, 2012-04-30 at 17:38 +0300, Michael S. Tsirkin wrote:
> The following makes 'x86info -r' dump hypervisor leaf cpu ids
> (for kvm this is signature+features) when running in a vm.
>
> On the guest we see the signature and the features:
> eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
>
> Hypervisor flag is checked to avoid output changes when not
> running on a VM.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Changes from v1:
> Make work on non KVM hypervisors (only KVM was tested).
> Avi Kivity said kvm will in the future report
> max HV leaf in eax. For now it reports eax = 0
> so add a work around for that.
>
> ---
>
> diff --git a/identify.c b/identify.c
> index 33f35de..a4a3763 100644
> --- a/identify.c
> +++ b/identify.c
> @@ -9,8 +9,8 @@
>
> void get_cpu_info_basics(struct cpudata *cpu)
> {
> - unsigned int maxi, maxei, vendor, address_bits;
> - unsigned int eax;
> + unsigned int maxi, maxei, maxhv, vendor, address_bits;
> + unsigned int eax, ebx, ecx;
>
> cpuid(cpu->number, 0, &maxi, &vendor, NULL, NULL);
> maxi &= 0xffff; /* The high-order word is non-zero on some Cyrix CPUs */
> @@ -19,7 +19,7 @@ void get_cpu_info_basics(struct cpudata *cpu)
> return;
>
> /* Everything that supports cpuid supports these. */
> - cpuid(cpu->number, 1, &eax, NULL, NULL, NULL);
> + cpuid(cpu->number, 1, &eax, &ebx, &ecx, NULL);
You probably want to check ebx, ecx, edx for the signatures of the
hypervisor's you are willing to support and which you know do something
sane with eax? Also it would be something worth reporting in its own
right?
BTW, according to arch/x86/include/asm/kvm_para.h unsurprisingly KVM has
a signature too 'KVMKVMKVM'.
> cpu->stepping = eax & 0xf;
> cpu->model = (eax >> 4) & 0xf;
> cpu->family = (eax >> 8) & 0xf;
> @@ -29,6 +29,19 @@ void get_cpu_info_basics(struct cpudata *cpu)
>
> cpuid(cpu->number, 0xC0000000, &maxei, NULL, NULL, NULL);
> cpu->maxei2 = maxei;
> + if (ecx & 0x80000000) {
> + cpuid(cpu->number, 0x40000000, &maxhv, NULL, NULL, NULL);
> + /*
> + * KVM up to linux 3.4 reports 0 as the max hypervisor leaf,
> + * where it really means 0x40000001.
This is something where I definitely think you want to check the
signature first.
Ian.
> + * Most (all?) hypervisors have at least one CPUID besides
> + * the vendor ID so assume that.
> + */
> + cpu->maxhv = maxhv ? maxhv : 0x40000001;
> + } else {
> + /* Suppress hypervisor cpuid unless running on a hypervisor */
> + cpu->maxhv = 0;
> + }
>
> cpuid(cpu->number, 0x80000008,&address_bits, NULL, NULL, NULL);
> cpu->phyaddr_bits = address_bits & 0xFF;
> diff --git a/x86info.c b/x86info.c
> index 22c4734..80cae36 100644
> --- a/x86info.c
> +++ b/x86info.c
> @@ -44,6 +44,10 @@ static void display_detailed_info(struct cpudata *cpu)
>
> if (cpu->maxei2 >=0xC0000000)
> dump_raw_cpuid(cpu->number, 0xC0000000, cpu->maxei2);
> +
> + if (cpu->maxhv >= 0x40000000)
> + dump_raw_cpuid(cpu->number, 0x40000000, cpu->maxhv);
> +
> }
>
> if (show_cacheinfo) {
> diff --git a/x86info.h b/x86info.h
> index 7d2a455..c4f5d81 100644
> --- a/x86info.h
> +++ b/x86info.h
> @@ -84,7 +84,7 @@ struct cpudata {
> unsigned int cachesize_trace;
> unsigned int phyaddr_bits;
> unsigned int viraddr_bits;
> - unsigned int cpuid_level, maxei, maxei2;
> + unsigned int cpuid_level, maxei, maxei2, maxhv;
> char name[CPU_NAME_LEN];
> enum connector connector;
> unsigned int flags_ecx;
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
--
Ian Campbell
Your own qualities will help prevent your advancement in the world.
^ permalink raw reply
* Re: [Xen-devel] x86info: dump kvm cpuid's
From: Ian Campbell @ 2012-05-01 12:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel@lists.xensource.com, kvm@vger.kernel.org,
pv-drivers@vmware.com, virtualization@lists.linux-foundation.org,
davej@redhat.com, devel@linuxdriverproject.org
In-Reply-To: <20120501105032.GA6654@redhat.com>
On Tue, 2012-05-01 at 11:50 +0100, Michael S. Tsirkin wrote:
> On Tue, May 01, 2012 at 11:29:04AM +0100, Ian Campbell wrote:
> > On Mon, 2012-04-30 at 10:38 +0100, Michael S. Tsirkin wrote:
> > > On Mon, Apr 30, 2012 at 11:43:19AM +0300, Gleb Natapov wrote:
> > > > On Sun, Apr 29, 2012 at 01:10:21PM +0300, Michael S. Tsirkin wrote:
> > > > > The following makes 'x86info -r' dump kvm cpu ids
> > > > > (signature+features) when running in a vm.
> > > > >
> > > > > On the guest we see the signature and the features:
> > > > > eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> > > > > eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
> > > > >
> > > > > On the host it just adds a couple of zero lines:
> > > > > eax in: 0x40000000, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > > > > eax in: 0x40000001, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > > > >
> > > > This is too KVM specific.
> > >
> > > That's what I have. I scratch my own itch.
> > >
> > > > Other hypervisors may use more cpuid leafs.
> > >
> > > But not less so no harm's done.
> > >
> > > > As far as I see Hyper-V uses 5 and use cpuid.0x40000000.eax as max cpuid
> > > > leaf available. Haven't checked Xen or VMWare.
> >
> > Xen does the same, documentation in the Xen public interfaces header:
> > http://xenbits.xen.org/docs/unstable/hypercall/include,public,arch-x86,cpuid.h.html.
>
> So ack to my patch?
I didn't see the patch, where should I be looking?
> > If compat mode for another h/v is enabled then those leaves will appear
> > at 0x40000000 and Xen's will be bumped up, so a fully Xen aware set of
> > drivers (or detection routine, etc) should check at 0x100 intervals
> > until 0x40010000 for the appropriate signatures (I realise that the docs
> > are somewhat lacking in this regard, I should cook up a patch).
> >
> > Ian.
>
> How does guest know that the data at 0x40000100 makes sense?
http://xenbits.xen.org/docs/unstable/hypercall/include,public,arch-x86,cpuid.h.html
EBX, ECX and EDX contain a signature "XenVMMXenVMM". I'm fairly certain
that hyperv has it's own magic number here.
Ian.
^ permalink raw reply
* Re: [Xen-devel] x86info: dump kvm cpuid's
From: Michael S. Tsirkin @ 2012-05-01 10:50 UTC (permalink / raw)
To: Ian Campbell
Cc: xen-devel@lists.xensource.com, kvm@vger.kernel.org,
pv-drivers@vmware.com, virtualization@lists.linux-foundation.org,
davej@redhat.com, devel@linuxdriverproject.org
In-Reply-To: <1335868144.6038.89.camel@zakaz.uk.xensource.com>
On Tue, May 01, 2012 at 11:29:04AM +0100, Ian Campbell wrote:
> On Mon, 2012-04-30 at 10:38 +0100, Michael S. Tsirkin wrote:
> > On Mon, Apr 30, 2012 at 11:43:19AM +0300, Gleb Natapov wrote:
> > > On Sun, Apr 29, 2012 at 01:10:21PM +0300, Michael S. Tsirkin wrote:
> > > > The following makes 'x86info -r' dump kvm cpu ids
> > > > (signature+features) when running in a vm.
> > > >
> > > > On the guest we see the signature and the features:
> > > > eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> > > > eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
> > > >
> > > > On the host it just adds a couple of zero lines:
> > > > eax in: 0x40000000, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > > > eax in: 0x40000001, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > > >
> > > This is too KVM specific.
> >
> > That's what I have. I scratch my own itch.
> >
> > > Other hypervisors may use more cpuid leafs.
> >
> > But not less so no harm's done.
> >
> > > As far as I see Hyper-V uses 5 and use cpuid.0x40000000.eax as max cpuid
> > > leaf available. Haven't checked Xen or VMWare.
>
> Xen does the same, documentation in the Xen public interfaces header:
> http://xenbits.xen.org/docs/unstable/hypercall/include,public,arch-x86,cpuid.h.html.
So ack to my patch?
> If compat mode for another h/v is enabled then those leaves will appear
> at 0x40000000 and Xen's will be bumped up, so a fully Xen aware set of
> drivers (or detection routine, etc) should check at 0x100 intervals
> until 0x40010000 for the appropriate signatures (I realise that the docs
> are somewhat lacking in this regard, I should cook up a patch).
>
> Ian.
How does guest know that the data at 0x40000100 makes sense?
--
MST
^ permalink raw reply
* Re: [Xen-devel] x86info: dump kvm cpuid's
From: Ian Campbell @ 2012-05-01 10:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel@lists.xensource.com, kvm@vger.kernel.org,
pv-drivers@vmware.com, virtualization@lists.linux-foundation.org,
davej@redhat.com, devel@linuxdriverproject.org
In-Reply-To: <20120430093811.GC5414@redhat.com>
On Mon, 2012-04-30 at 10:38 +0100, Michael S. Tsirkin wrote:
> On Mon, Apr 30, 2012 at 11:43:19AM +0300, Gleb Natapov wrote:
> > On Sun, Apr 29, 2012 at 01:10:21PM +0300, Michael S. Tsirkin wrote:
> > > The following makes 'x86info -r' dump kvm cpu ids
> > > (signature+features) when running in a vm.
> > >
> > > On the guest we see the signature and the features:
> > > eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> > > eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
> > >
> > > On the host it just adds a couple of zero lines:
> > > eax in: 0x40000000, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > > eax in: 0x40000001, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > >
> > This is too KVM specific.
>
> That's what I have. I scratch my own itch.
>
> > Other hypervisors may use more cpuid leafs.
>
> But not less so no harm's done.
>
> > As far as I see Hyper-V uses 5 and use cpuid.0x40000000.eax as max cpuid
> > leaf available. Haven't checked Xen or VMWare.
Xen does the same, documentation in the Xen public interfaces header:
http://xenbits.xen.org/docs/unstable/hypercall/include,public,arch-x86,cpuid.h.html.
If compat mode for another h/v is enabled then those leaves will appear
at 0x40000000 and Xen's will be bumped up, so a fully Xen aware set of
drivers (or detection routine, etc) should check at 0x100 intervals
until 0x40010000 for the appropriate signatures (I realise that the docs
are somewhat lacking in this regard, I should cook up a patch).
Ian.
^ permalink raw reply
* Re: [PATCHv2] x86info: dump kvm cpuid's
From: Gleb Natapov @ 2012-04-30 15:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, kvm, pv-drivers, virtualization, devel, davej
In-Reply-To: <20120430143835.GA10190@redhat.com>
On Mon, Apr 30, 2012 at 05:38:35PM +0300, Michael S. Tsirkin wrote:
> The following makes 'x86info -r' dump hypervisor leaf cpu ids
> (for kvm this is signature+features) when running in a vm.
>
> On the guest we see the signature and the features:
> eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
>
> Hypervisor flag is checked to avoid output changes when not
> running on a VM.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
Looks good to me.
> Changes from v1:
> Make work on non KVM hypervisors (only KVM was tested).
> Avi Kivity said kvm will in the future report
> max HV leaf in eax. For now it reports eax = 0
> so add a work around for that.
>
> ---
>
> diff --git a/identify.c b/identify.c
> index 33f35de..a4a3763 100644
> --- a/identify.c
> +++ b/identify.c
> @@ -9,8 +9,8 @@
>
> void get_cpu_info_basics(struct cpudata *cpu)
> {
> - unsigned int maxi, maxei, vendor, address_bits;
> - unsigned int eax;
> + unsigned int maxi, maxei, maxhv, vendor, address_bits;
> + unsigned int eax, ebx, ecx;
>
> cpuid(cpu->number, 0, &maxi, &vendor, NULL, NULL);
> maxi &= 0xffff; /* The high-order word is non-zero on some Cyrix CPUs */
> @@ -19,7 +19,7 @@ void get_cpu_info_basics(struct cpudata *cpu)
> return;
>
> /* Everything that supports cpuid supports these. */
> - cpuid(cpu->number, 1, &eax, NULL, NULL, NULL);
> + cpuid(cpu->number, 1, &eax, &ebx, &ecx, NULL);
> cpu->stepping = eax & 0xf;
> cpu->model = (eax >> 4) & 0xf;
> cpu->family = (eax >> 8) & 0xf;
> @@ -29,6 +29,19 @@ void get_cpu_info_basics(struct cpudata *cpu)
>
> cpuid(cpu->number, 0xC0000000, &maxei, NULL, NULL, NULL);
> cpu->maxei2 = maxei;
> + if (ecx & 0x80000000) {
> + cpuid(cpu->number, 0x40000000, &maxhv, NULL, NULL, NULL);
> + /*
> + * KVM up to linux 3.4 reports 0 as the max hypervisor leaf,
> + * where it really means 0x40000001.
> + * Most (all?) hypervisors have at least one CPUID besides
> + * the vendor ID so assume that.
> + */
> + cpu->maxhv = maxhv ? maxhv : 0x40000001;
> + } else {
> + /* Suppress hypervisor cpuid unless running on a hypervisor */
> + cpu->maxhv = 0;
> + }
>
> cpuid(cpu->number, 0x80000008,&address_bits, NULL, NULL, NULL);
> cpu->phyaddr_bits = address_bits & 0xFF;
> diff --git a/x86info.c b/x86info.c
> index 22c4734..80cae36 100644
> --- a/x86info.c
> +++ b/x86info.c
> @@ -44,6 +44,10 @@ static void display_detailed_info(struct cpudata *cpu)
>
> if (cpu->maxei2 >=0xC0000000)
> dump_raw_cpuid(cpu->number, 0xC0000000, cpu->maxei2);
> +
> + if (cpu->maxhv >= 0x40000000)
> + dump_raw_cpuid(cpu->number, 0x40000000, cpu->maxhv);
> +
> }
>
> if (show_cacheinfo) {
> diff --git a/x86info.h b/x86info.h
> index 7d2a455..c4f5d81 100644
> --- a/x86info.h
> +++ b/x86info.h
> @@ -84,7 +84,7 @@ struct cpudata {
> unsigned int cachesize_trace;
> unsigned int phyaddr_bits;
> unsigned int viraddr_bits;
> - unsigned int cpuid_level, maxei, maxei2;
> + unsigned int cpuid_level, maxei, maxei2, maxhv;
> char name[CPU_NAME_LEN];
> enum connector connector;
> unsigned int flags_ecx;
--
Gleb.
^ permalink raw reply
* [PATCHv2] x86info: dump kvm cpuid's
From: Michael S. Tsirkin @ 2012-04-30 14:38 UTC (permalink / raw)
To: davej; +Cc: xen-devel, kvm, pv-drivers, virtualization, devel
The following makes 'x86info -r' dump hypervisor leaf cpu ids
(for kvm this is signature+features) when running in a vm.
On the guest we see the signature and the features:
eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
Hypervisor flag is checked to avoid output changes when not
running on a VM.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Changes from v1:
Make work on non KVM hypervisors (only KVM was tested).
Avi Kivity said kvm will in the future report
max HV leaf in eax. For now it reports eax = 0
so add a work around for that.
---
diff --git a/identify.c b/identify.c
index 33f35de..a4a3763 100644
--- a/identify.c
+++ b/identify.c
@@ -9,8 +9,8 @@
void get_cpu_info_basics(struct cpudata *cpu)
{
- unsigned int maxi, maxei, vendor, address_bits;
- unsigned int eax;
+ unsigned int maxi, maxei, maxhv, vendor, address_bits;
+ unsigned int eax, ebx, ecx;
cpuid(cpu->number, 0, &maxi, &vendor, NULL, NULL);
maxi &= 0xffff; /* The high-order word is non-zero on some Cyrix CPUs */
@@ -19,7 +19,7 @@ void get_cpu_info_basics(struct cpudata *cpu)
return;
/* Everything that supports cpuid supports these. */
- cpuid(cpu->number, 1, &eax, NULL, NULL, NULL);
+ cpuid(cpu->number, 1, &eax, &ebx, &ecx, NULL);
cpu->stepping = eax & 0xf;
cpu->model = (eax >> 4) & 0xf;
cpu->family = (eax >> 8) & 0xf;
@@ -29,6 +29,19 @@ void get_cpu_info_basics(struct cpudata *cpu)
cpuid(cpu->number, 0xC0000000, &maxei, NULL, NULL, NULL);
cpu->maxei2 = maxei;
+ if (ecx & 0x80000000) {
+ cpuid(cpu->number, 0x40000000, &maxhv, NULL, NULL, NULL);
+ /*
+ * KVM up to linux 3.4 reports 0 as the max hypervisor leaf,
+ * where it really means 0x40000001.
+ * Most (all?) hypervisors have at least one CPUID besides
+ * the vendor ID so assume that.
+ */
+ cpu->maxhv = maxhv ? maxhv : 0x40000001;
+ } else {
+ /* Suppress hypervisor cpuid unless running on a hypervisor */
+ cpu->maxhv = 0;
+ }
cpuid(cpu->number, 0x80000008,&address_bits, NULL, NULL, NULL);
cpu->phyaddr_bits = address_bits & 0xFF;
diff --git a/x86info.c b/x86info.c
index 22c4734..80cae36 100644
--- a/x86info.c
+++ b/x86info.c
@@ -44,6 +44,10 @@ static void display_detailed_info(struct cpudata *cpu)
if (cpu->maxei2 >=0xC0000000)
dump_raw_cpuid(cpu->number, 0xC0000000, cpu->maxei2);
+
+ if (cpu->maxhv >= 0x40000000)
+ dump_raw_cpuid(cpu->number, 0x40000000, cpu->maxhv);
+
}
if (show_cacheinfo) {
diff --git a/x86info.h b/x86info.h
index 7d2a455..c4f5d81 100644
--- a/x86info.h
+++ b/x86info.h
@@ -84,7 +84,7 @@ struct cpudata {
unsigned int cachesize_trace;
unsigned int phyaddr_bits;
unsigned int viraddr_bits;
- unsigned int cpuid_level, maxei, maxei2;
+ unsigned int cpuid_level, maxei, maxei2, maxhv;
char name[CPU_NAME_LEN];
enum connector connector;
unsigned int flags_ecx;
^ permalink raw reply related
* Re: x86info: dump kvm cpuid's
From: Gleb Natapov @ 2012-04-30 9:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, kvm, pv-drivers, virtualization, devel, davej
In-Reply-To: <20120430093811.GC5414@redhat.com>
On Mon, Apr 30, 2012 at 12:38:13PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 30, 2012 at 11:43:19AM +0300, Gleb Natapov wrote:
> > On Sun, Apr 29, 2012 at 01:10:21PM +0300, Michael S. Tsirkin wrote:
> > > The following makes 'x86info -r' dump kvm cpu ids
> > > (signature+features) when running in a vm.
> > >
> > > On the guest we see the signature and the features:
> > > eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> > > eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
> > >
> > > On the host it just adds a couple of zero lines:
> > > eax in: 0x40000000, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > > eax in: 0x40000001, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > >
> > This is too KVM specific.
>
> That's what I have. I scratch my own itch.
>
> > Other hypervisors may use more cpuid leafs.
>
> But not less so no harm's done.
>
> > As far as I see Hyper-V uses 5 and use cpuid.0x40000000.eax as max cpuid
> > leaf available. Haven't checked Xen or VMWare.
>
> I don't think guessing at the CPU behaviour from linux source
> is the right thing to do.
>
That is guessing from Hyper-V specification. The best kind of guess.
http://msdn.microsoft.com/en-us/library/windows/hardware/ff542700%28v=vs.85%29.aspx
> I Cc'd some addresses found in MAINTAINERS in the linux
> kernel. This will give more people the opportunity
> to ask for their stuff to be added, if they care.
>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > ---
> > >
> > > Dave - not sure whether there's a mailing list for x86info.
> > > The patch is on top of the master branch in
> > > git://git.codemonkey.org.uk/x86info.git
> > >
> > > Thanks!
> > >
> > > diff --git a/x86info.c b/x86info.c
> > > index 22c4734..dee5ed1 100644
> > > --- a/x86info.c
> > > +++ b/x86info.c
> > > @@ -44,6 +44,7 @@ static void display_detailed_info(struct cpudata *cpu)
> > >
> > > if (cpu->maxei2 >=0xC0000000)
> > > dump_raw_cpuid(cpu->number, 0xC0000000, cpu->maxei2);
> > > + dump_raw_cpuid(cpu->number, 0x40000000, 0x40000001);
> > > }
> > >
> > > if (show_cacheinfo) {
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply
* Re: x86info: dump kvm cpuid's
From: Michael S. Tsirkin @ 2012-04-30 9:38 UTC (permalink / raw)
To: Gleb Natapov; +Cc: xen-devel, kvm, pv-drivers, virtualization, devel, davej
In-Reply-To: <20120430084319.GE15413@redhat.com>
On Mon, Apr 30, 2012 at 11:43:19AM +0300, Gleb Natapov wrote:
> On Sun, Apr 29, 2012 at 01:10:21PM +0300, Michael S. Tsirkin wrote:
> > The following makes 'x86info -r' dump kvm cpu ids
> > (signature+features) when running in a vm.
> >
> > On the guest we see the signature and the features:
> > eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> > eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
> >
> > On the host it just adds a couple of zero lines:
> > eax in: 0x40000000, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> > eax in: 0x40000001, eax = 00000000 ebx = 00000000 ecx = 00000000 edx = 00000000
> >
> This is too KVM specific.
That's what I have. I scratch my own itch.
> Other hypervisors may use more cpuid leafs.
But not less so no harm's done.
> As far as I see Hyper-V uses 5 and use cpuid.0x40000000.eax as max cpuid
> leaf available. Haven't checked Xen or VMWare.
I don't think guessing at the CPU behaviour from linux source
is the right thing to do.
I Cc'd some addresses found in MAINTAINERS in the linux
kernel. This will give more people the opportunity
to ask for their stuff to be added, if they care.
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > Dave - not sure whether there's a mailing list for x86info.
> > The patch is on top of the master branch in
> > git://git.codemonkey.org.uk/x86info.git
> >
> > Thanks!
> >
> > diff --git a/x86info.c b/x86info.c
> > index 22c4734..dee5ed1 100644
> > --- a/x86info.c
> > +++ b/x86info.c
> > @@ -44,6 +44,7 @@ static void display_detailed_info(struct cpudata *cpu)
> >
> > if (cpu->maxei2 >=0xC0000000)
> > dump_raw_cpuid(cpu->number, 0xC0000000, cpu->maxei2);
> > + dump_raw_cpuid(cpu->number, 0x40000000, 0x40000001);
> > }
> >
> > if (show_cacheinfo) {
>
> --
> Gleb.
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Gleb Natapov @ 2012-04-30 8:38 UTC (permalink / raw)
To: Avi Kivity
Cc: X86, Jeremy Fitzhardinge, Greg Kroah-Hartman, KVM,
Konrad Rzeszutek Wilk, linux-doc, LKML, Raghavendra K T,
Virtualization, Ingo Molnar, Srivatsa Vaddagiri, Sasha Levin,
H. Peter Anvin, Xen, Stefano Stabellini
In-Reply-To: <4F9E4BCA.5030604@redhat.com>
On Mon, Apr 30, 2012 at 11:22:34AM +0300, Avi Kivity wrote:
> On 04/29/2012 04:52 PM, Gleb Natapov wrote:
> > On Sun, Apr 29, 2012 at 04:26:21PM +0300, Avi Kivity wrote:
> > > On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > > > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > > > > can use one of reserved delivery modes as PV delivery mode. We will
> > > > > > disallow guest to trigger it through apic interface, so this will not be
> > > > > > part of ABI and can be changed at will.
> > > > > >
> > > > >
> > > > > I'm not thrilled about this. Those delivery modes will eventually
> > > > > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > > > > among implementations.
> > > > >
> > > > This is only internal implementation. If they become unreserved we will
> > > > use something else.
> > > >
> > >
> > > Yeah, I'm thinking of that time. Why do something temporary and fragile?
> > >
> > Why is it fragile? Just by unreserving the value Intel will not break
> > KVM. Only when KVM will implement apic feature that unreserves the value
> > we will have to change internal implementation and use another value,
> > but this will be done by the same patch that does unreserving. The
> > unreserving may even never happen.
>
> Some remains of that may leak somewhere.
I do not see where. APIC code should #GP if a guest attempts to set
reserved values through APIC interface, or at least ignore them.
> Why not add an extra
> parameter?
Yes, we can add extra parameter to "struct kvm_lapic_irq" and propagate it to
__apic_accept_irq(). We can do that now, or when Intel unreserve all
reserved values. I prefer the later since I do not believe it will
happen in observable feature.
> Or do something like
>
> kvm_for_each_apic_dest(vcpu, apic_destination) {
> ...
> }
>
> That can be reused in both the apic code and pv kick.
>
That's exactly what kvm_irq_delivery_to_apic() is.
> > Meanwhile kvm_irq_delivery_to_apic()
> > will likely be optimized to use hash for unicast delivery and unhalt
> > hypercall will benefit from it immediately.
>
> Overloading delivery mode is not the only way to achieve sharing.
>
It is simplest and most straightforward with no demonstratable drawbacks :)
Adding parameter to "struct kvm_lapic_irq" is next best thing.
--
Gleb.
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Avi Kivity @ 2012-04-30 8:22 UTC (permalink / raw)
To: Gleb Natapov
Cc: X86, Jeremy Fitzhardinge, Greg Kroah-Hartman, KVM,
Konrad Rzeszutek Wilk, linux-doc, LKML, Raghavendra K T,
Virtualization, Ingo Molnar, Srivatsa Vaddagiri, Sasha Levin,
H. Peter Anvin, Xen, Stefano Stabellini
In-Reply-To: <20120429135227.GC15413@redhat.com>
On 04/29/2012 04:52 PM, Gleb Natapov wrote:
> On Sun, Apr 29, 2012 at 04:26:21PM +0300, Avi Kivity wrote:
> > On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > > > can use one of reserved delivery modes as PV delivery mode. We will
> > > > > disallow guest to trigger it through apic interface, so this will not be
> > > > > part of ABI and can be changed at will.
> > > > >
> > > >
> > > > I'm not thrilled about this. Those delivery modes will eventually
> > > > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > > > among implementations.
> > > >
> > > This is only internal implementation. If they become unreserved we will
> > > use something else.
> > >
> >
> > Yeah, I'm thinking of that time. Why do something temporary and fragile?
> >
> Why is it fragile? Just by unreserving the value Intel will not break
> KVM. Only when KVM will implement apic feature that unreserves the value
> we will have to change internal implementation and use another value,
> but this will be done by the same patch that does unreserving. The
> unreserving may even never happen.
Some remains of that may leak somewhere. Why not add an extra
parameter? Or do something like
kvm_for_each_apic_dest(vcpu, apic_destination) {
...
}
That can be reused in both the apic code and pv kick.
> Meanwhile kvm_irq_delivery_to_apic()
> will likely be optimized to use hash for unicast delivery and unhalt
> hypercall will benefit from it immediately.
Overloading delivery mode is not the only way to achieve sharing.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Avi Kivity @ 2012-04-30 8:19 UTC (permalink / raw)
To: Raghavendra K T
Cc: Jeremy Fitzhardinge, X86, KVM, Konrad Rzeszutek Wilk, linux-doc,
LKML, Greg Kroah-Hartman, Virtualization, Ingo Molnar,
Srivatsa Vaddagiri, Sasha Levin, H. Peter Anvin, Xen,
Stefano Stabellini
In-Reply-To: <4F9E42F6.8050108@linux.vnet.ibm.com>
On 04/30/2012 10:44 AM, Raghavendra K T wrote:
>> Hm, what about reusing KVM_REQ_UNHALT?
>>
>
>
> Yes, I had experimented this for some time without success.
> For e.g. having
> make_request(KVM_REQ_UNHALT, vcpu) directly from kick hypercall.
>
> It would still need a flag. (did not get any alternative so far except
> the workaround posted in V4) :(
>
Okay.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH RFC V6 2/5] kvm : Fold pv_unhalt flag into GET_MP_STATE ioctl to aid migration
From: Raghavendra K T @ 2012-04-30 7:45 UTC (permalink / raw)
To: Avi Kivity
Cc: Jeremy Fitzhardinge, X86, KVM, linux-doc, LKML,
Greg Kroah-Hartman, Konrad Rzeszutek Wilk, Ingo Molnar,
Srivatsa Vaddagiri, Sasha Levin, H. Peter Anvin, Virtualization,
Xen, Stefano Stabellini
In-Reply-To: <4F9D41DE.3080100@redhat.com>
On 04/29/2012 06:57 PM, Avi Kivity wrote:
> On 04/23/2012 01:00 PM, Raghavendra K T wrote:
>> From: Raghavendra K T<raghavendra.kt@linux.vnet.ibm.com>
>>
>> Signed-off-by: Raghavendra K T<raghavendra.kt@linux.vnet.ibm.com>
>> ---
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 6faa550..7354c1b 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5691,7 +5691,9 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
>> int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
>> struct kvm_mp_state *mp_state)
>> {
>> - mp_state->mp_state = vcpu->arch.mp_state;
>> + mp_state->mp_state = (vcpu->arch.mp_state == KVM_MP_STATE_HALTED&&
>> + vcpu->arch.pv.pv_unhalted) ?
>> + KVM_MP_STATE_RUNNABLE : vcpu->arch.mp_state;
>> return 0;
>> }
>
> Not pretty. Suggest rewriting using an if.
>
Ok.. 'll rewrite.
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Raghavendra K T @ 2012-04-30 7:44 UTC (permalink / raw)
To: Avi Kivity
Cc: Jeremy Fitzhardinge, X86, KVM, Konrad Rzeszutek Wilk, linux-doc,
LKML, Greg Kroah-Hartman, Virtualization, Ingo Molnar,
Srivatsa Vaddagiri, Sasha Levin, H. Peter Anvin, Xen,
Stefano Stabellini
In-Reply-To: <4F9D415B.7010103@redhat.com>
Thanks Avi, for the review.
On 04/29/2012 06:55 PM, Avi Kivity wrote:
> On 04/23/2012 12:59 PM, Raghavendra K T wrote:
>> From: Srivatsa Vaddagiri<vatsa@linux.vnet.ibm.com>
>>
>> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>>
>> The presence of these hypercalls is indicated to guest via
>> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>>
>> #endif
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index e216ba0..dad475b 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -481,6 +481,10 @@ struct kvm_vcpu_arch {
>> u64 length;
>> u64 status;
>> } osvw;
>> + /* pv related host specific info */
>> + struct {
>> + int pv_unhalted;
>> + } pv;
>> };
>
> 'bool'. Or maybe push into vcpu->requests.
Ok. I think you meant
+ struct {
+ bool pv_unhalted;
+ } pv;
and as discussed in old series (V4), cleaner implementation having
vcpu request, would still need a flag to prevent vcpu hang, so back to
having one flag.
>
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 4044ce0..7fc9be6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
>> case KVM_CAP_ASYNC_PF:
>> case KVM_CAP_GET_TSC_KHZ:
>> case KVM_CAP_PCI_2_3:
>> + case KVM_CAP_PV_UNHALT:
>> r = 1;
>> break;
>> case KVM_CAP_COALESCED_MMIO:
>
> Redundant, since we can infer this from KVM_GET_SUPPORTED_CPUID. But
> please indicate this in the documentation.
>
Ok. will mention that in documentation added for KVM_CAP_PV_UNHALT.
>>
>> +/*
>> + * kvm_pv_kick_cpu_op: Kick a vcpu.
>> + *
>> + * @apicid - apicid of vcpu to be kicked.
>> + */
>> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
>> +{
>> + struct kvm_vcpu *vcpu = NULL;
>> + int i;
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + if (!kvm_apic_present(vcpu))
>> + continue;
>> +
>> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
>> + break;
>> + }
>> + if (vcpu) {
>> + /*
>> + * Setting unhalt flag here can result in spurious runnable
>> + * state when unhalt reset does not happen in vcpu_block.
>> + * But that is harmless since that should soon result in halt.
>> + */
>> + vcpu->arch.pv.pv_unhalted = 1;
>> + /* We need everybody see unhalt before vcpu unblocks */
>> + smp_mb();
>
> smp_wmb().
>
Done.
>> + kvm_vcpu_kick(vcpu);
>> + }
>> +}
>> +
>>
>> /*
>> * hypercalls use architecture specific
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 42b7393..edf56d4 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>> prepare_to_wait(&vcpu->wq,&wait, TASK_INTERRUPTIBLE);
>>
>> if (kvm_arch_vcpu_runnable(vcpu)) {
>> + /*
>> + * This is the only safe place to reset unhalt flag.
>> + * otherwise it results in loosing the notification
>> + * which eventually can result in vcpu hangs.
>> + */
>> + kvm_arch_vcpu_reset_pv_unhalted(vcpu);
>> + /* preventing reordering should be enough here */
>> + barrier();
>> kvm_make_request(KVM_REQ_UNHALT, vcpu);
>> break;
>> }
>>
>
> Hm, what about reusing KVM_REQ_UNHALT?
>
Yes, I had experimented this for some time without success.
For e.g. having
make_request(KVM_REQ_UNHALT, vcpu) directly from kick hypercall.
It would still need a flag. (did not get any alternative so far except
the workaround posted in V4) :(
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Gleb Natapov @ 2012-04-29 13:52 UTC (permalink / raw)
To: Avi Kivity
Cc: X86, Jeremy Fitzhardinge, Greg Kroah-Hartman, KVM,
Konrad Rzeszutek Wilk, linux-doc, LKML, Raghavendra K T,
Virtualization, Ingo Molnar, Srivatsa Vaddagiri, Sasha Levin,
H. Peter Anvin, Xen, Stefano Stabellini
In-Reply-To: <4F9D417D.1050105@redhat.com>
On Sun, Apr 29, 2012 at 04:26:21PM +0300, Avi Kivity wrote:
> On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > > can use one of reserved delivery modes as PV delivery mode. We will
> > > > disallow guest to trigger it through apic interface, so this will not be
> > > > part of ABI and can be changed at will.
> > > >
> > >
> > > I'm not thrilled about this. Those delivery modes will eventually
> > > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > > among implementations.
> > >
> > This is only internal implementation. If they become unreserved we will
> > use something else.
> >
>
> Yeah, I'm thinking of that time. Why do something temporary and fragile?
>
Why is it fragile? Just by unreserving the value Intel will not break
KVM. Only when KVM will implement apic feature that unreserves the value
we will have to change internal implementation and use another value,
but this will be done by the same patch that does unreserving. The
unreserving may even never happen. Meanwhile kvm_irq_delivery_to_apic()
will likely be optimized to use hash for unicast delivery and unhalt
hypercall will benefit from it immediately.
--
Gleb.
^ permalink raw reply
* Re: [PATCH RFC V6 2/5] kvm : Fold pv_unhalt flag into GET_MP_STATE ioctl to aid migration
From: Avi Kivity @ 2012-04-29 13:27 UTC (permalink / raw)
To: Raghavendra K T
Cc: Jeremy Fitzhardinge, X86, KVM, linux-doc, LKML,
Greg Kroah-Hartman, Konrad Rzeszutek Wilk, Ingo Molnar,
Srivatsa Vaddagiri, Sasha Levin, H. Peter Anvin, Virtualization,
Xen, Stefano Stabellini
In-Reply-To: <20120423100002.30893.60584.sendpatchset@codeblue.in.ibm.com>
On 04/23/2012 01:00 PM, Raghavendra K T wrote:
> From: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
>
> Signed-off-by: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6faa550..7354c1b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5691,7 +5691,9 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
> int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
> struct kvm_mp_state *mp_state)
> {
> - mp_state->mp_state = vcpu->arch.mp_state;
> + mp_state->mp_state = (vcpu->arch.mp_state == KVM_MP_STATE_HALTED &&
> + vcpu->arch.pv.pv_unhalted) ?
> + KVM_MP_STATE_RUNNABLE : vcpu->arch.mp_state;
> return 0;
> }
Not pretty. Suggest rewriting using an if.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Avi Kivity @ 2012-04-29 13:26 UTC (permalink / raw)
To: Gleb Natapov
Cc: X86, Jeremy Fitzhardinge, Greg Kroah-Hartman, KVM,
Konrad Rzeszutek Wilk, linux-doc, LKML, Raghavendra K T,
Virtualization, Ingo Molnar, Srivatsa Vaddagiri, Sasha Levin,
H. Peter Anvin, Xen, Stefano Stabellini
In-Reply-To: <20120429132030.GB15413@redhat.com>
On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > can use one of reserved delivery modes as PV delivery mode. We will
> > > disallow guest to trigger it through apic interface, so this will not be
> > > part of ABI and can be changed at will.
> > >
> >
> > I'm not thrilled about this. Those delivery modes will eventually
> > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > among implementations.
> >
> This is only internal implementation. If they become unreserved we will
> use something else.
>
Yeah, I'm thinking of that time. Why do something temporary and fragile?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Avi Kivity @ 2012-04-29 13:25 UTC (permalink / raw)
To: Raghavendra K T
Cc: Jeremy Fitzhardinge, X86, KVM, Konrad Rzeszutek Wilk, linux-doc,
LKML, Greg Kroah-Hartman, Virtualization, Ingo Molnar,
Srivatsa Vaddagiri, Sasha Levin, H. Peter Anvin, Xen,
Stefano Stabellini
In-Reply-To: <20120423095947.30893.84029.sendpatchset@codeblue.in.ibm.com>
On 04/23/2012 12:59 PM, Raghavendra K T wrote:
> From: Srivatsa Vaddagiri <vatsa@linux.vnet.ibm.com>
>
> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>
> The presence of these hypercalls is indicated to guest via
> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>
> #endif
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e216ba0..dad475b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -481,6 +481,10 @@ struct kvm_vcpu_arch {
> u64 length;
> u64 status;
> } osvw;
> + /* pv related host specific info */
> + struct {
> + int pv_unhalted;
> + } pv;
> };
'bool'. Or maybe push into vcpu->requests.
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4044ce0..7fc9be6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> case KVM_CAP_ASYNC_PF:
> case KVM_CAP_GET_TSC_KHZ:
> case KVM_CAP_PCI_2_3:
> + case KVM_CAP_PV_UNHALT:
> r = 1;
> break;
> case KVM_CAP_COALESCED_MMIO:
Redundant, since we can infer this from KVM_GET_SUPPORTED_CPUID. But
please indicate this in the documentation.
>
> +/*
> + * kvm_pv_kick_cpu_op: Kick a vcpu.
> + *
> + * @apicid - apicid of vcpu to be kicked.
> + */
> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> +{
> + struct kvm_vcpu *vcpu = NULL;
> + int i;
> +
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + if (!kvm_apic_present(vcpu))
> + continue;
> +
> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> + break;
> + }
> + if (vcpu) {
> + /*
> + * Setting unhalt flag here can result in spurious runnable
> + * state when unhalt reset does not happen in vcpu_block.
> + * But that is harmless since that should soon result in halt.
> + */
> + vcpu->arch.pv.pv_unhalted = 1;
> + /* We need everybody see unhalt before vcpu unblocks */
> + smp_mb();
smp_wmb().
> + kvm_vcpu_kick(vcpu);
> + }
> +}
> +
>
> /*
> * hypercalls use architecture specific
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 42b7393..edf56d4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>
> if (kvm_arch_vcpu_runnable(vcpu)) {
> + /*
> + * This is the only safe place to reset unhalt flag.
> + * otherwise it results in loosing the notification
> + * which eventually can result in vcpu hangs.
> + */
> + kvm_arch_vcpu_reset_pv_unhalted(vcpu);
> + /* preventing reordering should be enough here */
> + barrier();
> kvm_make_request(KVM_REQ_UNHALT, vcpu);
> break;
> }
>
Hm, what about reusing KVM_REQ_UNHALT?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Gleb Natapov @ 2012-04-29 13:20 UTC (permalink / raw)
To: Avi Kivity
Cc: X86, Jeremy Fitzhardinge, Greg Kroah-Hartman, KVM,
Konrad Rzeszutek Wilk, linux-doc, LKML, Raghavendra K T,
Virtualization, Ingo Molnar, Srivatsa Vaddagiri, Sasha Levin,
H. Peter Anvin, Xen, Stefano Stabellini
In-Reply-To: <4F9D3F8B.9010805@redhat.com>
On Sun, Apr 29, 2012 at 04:18:03PM +0300, Avi Kivity wrote:
> On 04/24/2012 12:59 PM, Gleb Natapov wrote:
> > >
> > > +/*
> > > + * kvm_pv_kick_cpu_op: Kick a vcpu.
> > > + *
> > > + * @apicid - apicid of vcpu to be kicked.
> > > + */
> > > +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> > > +{
> > > + struct kvm_vcpu *vcpu = NULL;
> > > + int i;
> > > +
> > > + kvm_for_each_vcpu(i, vcpu, kvm) {
> > > + if (!kvm_apic_present(vcpu))
> > > + continue;
> > > +
> > > + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> > > + break;
> > > + }
> > > + if (vcpu) {
> > > + /*
> > > + * Setting unhalt flag here can result in spurious runnable
> > > + * state when unhalt reset does not happen in vcpu_block.
> > > + * But that is harmless since that should soon result in halt.
> > > + */
> > > + vcpu->arch.pv.pv_unhalted = 1;
> > > + /* We need everybody see unhalt before vcpu unblocks */
> > > + smp_mb();
> > > + kvm_vcpu_kick(vcpu);
> > > + }
> > > +}
> > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > can use one of reserved delivery modes as PV delivery mode. We will
> > disallow guest to trigger it through apic interface, so this will not be
> > part of ABI and can be changed at will.
> >
>
> I'm not thrilled about this. Those delivery modes will eventually
> become unreserved. We can have a kvm_lookup_apic_id() that is shared
> among implementations.
>
This is only internal implementation. If they become unreserved we will
use something else.
--
Gleb.
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Avi Kivity @ 2012-04-29 13:18 UTC (permalink / raw)
To: Gleb Natapov
Cc: X86, Jeremy Fitzhardinge, Greg Kroah-Hartman, KVM,
Konrad Rzeszutek Wilk, linux-doc, LKML, Raghavendra K T,
Virtualization, Ingo Molnar, Srivatsa Vaddagiri, Sasha Levin,
H. Peter Anvin, Xen, Stefano Stabellini
In-Reply-To: <20120424095923.GS15413@redhat.com>
On 04/24/2012 12:59 PM, Gleb Natapov wrote:
> >
> > +/*
> > + * kvm_pv_kick_cpu_op: Kick a vcpu.
> > + *
> > + * @apicid - apicid of vcpu to be kicked.
> > + */
> > +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> > +{
> > + struct kvm_vcpu *vcpu = NULL;
> > + int i;
> > +
> > + kvm_for_each_vcpu(i, vcpu, kvm) {
> > + if (!kvm_apic_present(vcpu))
> > + continue;
> > +
> > + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> > + break;
> > + }
> > + if (vcpu) {
> > + /*
> > + * Setting unhalt flag here can result in spurious runnable
> > + * state when unhalt reset does not happen in vcpu_block.
> > + * But that is harmless since that should soon result in halt.
> > + */
> > + vcpu->arch.pv.pv_unhalted = 1;
> > + /* We need everybody see unhalt before vcpu unblocks */
> > + smp_mb();
> > + kvm_vcpu_kick(vcpu);
> > + }
> > +}
> This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> can use one of reserved delivery modes as PV delivery mode. We will
> disallow guest to trigger it through apic interface, so this will not be
> part of ABI and can be changed at will.
>
I'm not thrilled about this. Those delivery modes will eventually
become unreserved. We can have a kvm_lookup_apic_id() that is shared
among implementations.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [Xen-devel] [PATCH] drivers/video/xen-fbfront.c: add missing cleanup code
From: Florian Tobias Schandinat @ 2012-04-28 23:47 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Jeremy Fitzhardinge, xen-devel, kernel-janitors, linux-kernel,
virtualization, Julia Lawall, linux-fbdev
In-Reply-To: <20120426214222.GA11289@phenom.dumpdata.com>
Hi Konrad,
On 04/26/2012 09:42 PM, Konrad Rzeszutek Wilk wrote:
> On Sun, Apr 22, 2012 at 11:57:40AM +0200, Julia Lawall wrote:
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> The operations in the subsequent error-handling code appear to be also
>> useful here.
>
> How about doing it this way?
Looks good to me.
> Florian, are you OK me carrying this patch in my tree for Linus or would
> you prefer to do it?
Yes, I'm okay with you carrying this patch. Feel free to add
Acked-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Best regards,
Florian Tobias Schandinat
>
> commit a833fb9973b47cb30d1086e73f20b62a425bcd85
> Author: Julia Lawall <Julia.Lawall@lip6.fr>
> Date: Sun Apr 22 11:57:40 2012 +0200
>
> drivers/video/xen-fbfront.c: add missing cleanup code
>
> The operations in the subsequent error-handling code appear to be also
> useful here.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> [v1: Collapse some of the error handling functions]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
> index cb4529c..aa42160 100644
> --- a/drivers/video/xen-fbfront.c
> +++ b/drivers/video/xen-fbfront.c
> @@ -458,26 +458,31 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
> xenfb_init_shared_page(info, fb_info);
>
> ret = xenfb_connect_backend(dev, info);
> - if (ret < 0)
> - goto error;
> + if (ret < 0) {
> + xenbus_dev_fatal(dev, ret, "xenfb_connect_backend");
> + goto error_fb;
> + }
>
> ret = register_framebuffer(fb_info);
> if (ret) {
> - fb_deferred_io_cleanup(fb_info);
> - fb_dealloc_cmap(&fb_info->cmap);
> - framebuffer_release(fb_info);
> xenbus_dev_fatal(dev, ret, "register_framebuffer");
> - goto error;
> + goto error_fb;
> }
> info->fb_info = fb_info;
>
> xenfb_make_preferred_console();
> return 0;
>
> - error_nomem:
> - ret = -ENOMEM;
> - xenbus_dev_fatal(dev, ret, "allocating device memory");
> - error:
> +error_fb:
> + fb_deferred_io_cleanup(fb_info);
> + fb_dealloc_cmap(&fb_info->cmap);
> + framebuffer_release(fb_info);
> +error_nomem:
> + if (!ret) {
> + ret = -ENOMEM;
> + xenbus_dev_fatal(dev, ret, "allocating device memory");
> + }
> +error:
> xenfb_remove(dev);
> return ret;
> }
>
>
>
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> ---
>> Not tested.
>>
>> drivers/video/xen-fbfront.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
>> index cb4529c..b0bd59c 100644
>> --- a/drivers/video/xen-fbfront.c
>> +++ b/drivers/video/xen-fbfront.c
>> @@ -458,8 +458,13 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
>> xenfb_init_shared_page(info, fb_info);
>>
>> ret = xenfb_connect_backend(dev, info);
>> - if (ret < 0)
>> + if (ret < 0) {
>> + fb_deferred_io_cleanup(fb_info);
>> + fb_dealloc_cmap(&fb_info->cmap);
>> + framebuffer_release(fb_info);
>> + xenbus_dev_fatal(dev, ret, "xenfb_connect_backend");
>> goto error;
>> + }
>>
>> ret = register_framebuffer(fb_info);
>> if (ret) {
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Gleb Natapov @ 2012-04-27 15:53 UTC (permalink / raw)
To: Raghavendra K T
Cc: Jeremy Fitzhardinge, X86, KVM, Konrad Rzeszutek Wilk, linux-doc,
LKML, Greg Kroah-Hartman, Virtualization, Ingo Molnar,
Srivatsa Vaddagiri, Avi Kivity, H. Peter Anvin, Xen,
Stefano Stabellini, Sasha Levin
In-Reply-To: <4F9A78CF.7070005@linux.vnet.ibm.com>
On Fri, Apr 27, 2012 at 04:15:35PM +0530, Raghavendra K T wrote:
> On 04/24/2012 03:29 PM, Gleb Natapov wrote:
> >On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
> >>From: Srivatsa Vaddagiri<vatsa@linux.vnet.ibm.com>
> >>
> >>KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
> >>
> >>The presence of these hypercalls is indicated to guest via
> >>KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
> >>
> >>Signed-off-by: Srivatsa Vaddagiri<vatsa@linux.vnet.ibm.com>
> >>Signed-off-by: Suzuki Poulose<suzuki@in.ibm.com>
> >>Signed-off-by: Raghavendra K T<raghavendra.kt@linux.vnet.ibm.com>
> >>---
> [...]
> >>+/*
> >>+ * kvm_pv_kick_cpu_op: Kick a vcpu.
> >>+ *
> >>+ * @apicid - apicid of vcpu to be kicked.
> >>+ */
> >>+static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> >>+{
> >>+ struct kvm_vcpu *vcpu = NULL;
> >>+ int i;
> >>+
> >>+ kvm_for_each_vcpu(i, vcpu, kvm) {
> >>+ if (!kvm_apic_present(vcpu))
> >>+ continue;
> >>+
> >>+ if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> >>+ break;
> >>+ }
> >>+ if (vcpu) {
> >>+ /*
> >>+ * Setting unhalt flag here can result in spurious runnable
> >>+ * state when unhalt reset does not happen in vcpu_block.
> >>+ * But that is harmless since that should soon result in halt.
> >>+ */
> >>+ vcpu->arch.pv.pv_unhalted = 1;
> >>+ /* We need everybody see unhalt before vcpu unblocks */
> >>+ smp_mb();
> >>+ kvm_vcpu_kick(vcpu);
> >>+ }
> >>+}
> >This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> >can use one of reserved delivery modes as PV delivery mode. We will
> >disallow guest to trigger it through apic interface, so this will not be
> >part of ABI and can be changed at will.
> >
>
> I think it is interesting ( Perhaps more reasonable way of doing it).
> I am not too familiar with lapic source. So, pardon me if my
> questions are stupid.
>
> Something like below is what I deciphered from your suggestion which
> is working.
>
> kvm/x86.c
> =========
> kvm_pv_kick_cpu_op()
> {
>
> struct kvm_lapic_irq lapic_irq;
>
> lapic_irq.shorthand = 0;
> lapic_irq.dest_mode = 0;
> lapic_irq.dest_id = apicid;
>
> lapic_irq.delivery_mode = PV_DELIVERY_MODE;
> kvm_irq_delivery_to_apic(kvm, 0, &lapic_irq );
>
> }
>
> kvm/lapic.c
> ==========
> _apic_accept_irq()
> {
> ...
> case APIC_DM_REMRD:
> result = 1;
> vcpu->pv_unhalted = 1
> smp_mb();
> kvm_make_request(KVM_REQ_EVENT, vcpu);
> kvm_vcpu_kick(vcpu);
> break;
>
> ...
> }
>
> here using PV_DELIVERY_MODE = APIC_DM_REMRD, which was unused.
>
Yes, this is what I mean except that PV_DELIVERY_MODE should be
number defined as reserved by Intel spec.
> OR
> 1) Are you asking to remove vcpu->pv_unhalted flag and replace with an irq?
I would like to remove vcpu->pv_unhalted, but do not see how to do that,
so I do not asking that :)
> 2) are you talking about some reserved fields in struct local_apic instead
> of APIC_DM_REMRD what I have used above?
Delivery modes 011b and 111b are reserved. We can use one if them.
> [ Honestly, arch/x86/include/asm/apicdef.h had too much of info to
> digest :( ]
>
> 3) I am not sure about: disallow guest to trigger it through apic
> interface part also.(mean howto?)
I mean we should disallow guest to set delivery mode to reserved values
through apic interface.
> 4) And one more question, So you think it takes care of migration part
> (in case we are removing pv_unhalted flag)?
No, since I am not asking for removing pv_unhalted flag. I want to reuse
code that we already have to deliver the unhalt event.
>
> It would be helpful if you can give little more explanation/ pointer
> to Documentation.
>
> Ingo is keen to see whole ticketlock/Xen/KVM patch in one go.
> and anyhow since this does not cause any ABI change, I hope you
> don't mind if
> I do only the vcpu->pv_unhalted change you suggested now [ having
> pv_unhalted reset in vcpu_run if
> you meant something else than code I have above ], so that whole
> series get fair amount time for testing.
--
Gleb.
^ permalink raw reply
* Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks
From: Raghavendra K T @ 2012-04-27 10:45 UTC (permalink / raw)
To: Gleb Natapov
Cc: Jeremy Fitzhardinge, X86, KVM, Konrad Rzeszutek Wilk, linux-doc,
LKML, Greg Kroah-Hartman, Virtualization, Ingo Molnar,
Srivatsa Vaddagiri, Avi Kivity, H. Peter Anvin, Xen,
Stefano Stabellini, Sasha Levin
In-Reply-To: <20120424095923.GS15413@redhat.com>
On 04/24/2012 03:29 PM, Gleb Natapov wrote:
> On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
>> From: Srivatsa Vaddagiri<vatsa@linux.vnet.ibm.com>
>>
>> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>>
>> The presence of these hypercalls is indicated to guest via
>> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>>
>> Signed-off-by: Srivatsa Vaddagiri<vatsa@linux.vnet.ibm.com>
>> Signed-off-by: Suzuki Poulose<suzuki@in.ibm.com>
>> Signed-off-by: Raghavendra K T<raghavendra.kt@linux.vnet.ibm.com>
>> ---
[...]
>> +/*
>> + * kvm_pv_kick_cpu_op: Kick a vcpu.
>> + *
>> + * @apicid - apicid of vcpu to be kicked.
>> + */
>> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
>> +{
>> + struct kvm_vcpu *vcpu = NULL;
>> + int i;
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + if (!kvm_apic_present(vcpu))
>> + continue;
>> +
>> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
>> + break;
>> + }
>> + if (vcpu) {
>> + /*
>> + * Setting unhalt flag here can result in spurious runnable
>> + * state when unhalt reset does not happen in vcpu_block.
>> + * But that is harmless since that should soon result in halt.
>> + */
>> + vcpu->arch.pv.pv_unhalted = 1;
>> + /* We need everybody see unhalt before vcpu unblocks */
>> + smp_mb();
>> + kvm_vcpu_kick(vcpu);
>> + }
>> +}
> This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> can use one of reserved delivery modes as PV delivery mode. We will
> disallow guest to trigger it through apic interface, so this will not be
> part of ABI and can be changed at will.
>
I think it is interesting ( Perhaps more reasonable way of doing it).
I am not too familiar with lapic source. So, pardon me if my questions
are stupid.
Something like below is what I deciphered from your suggestion which is
working.
kvm/x86.c
=========
kvm_pv_kick_cpu_op()
{
struct kvm_lapic_irq lapic_irq;
lapic_irq.shorthand = 0;
lapic_irq.dest_mode = 0;
lapic_irq.dest_id = apicid;
lapic_irq.delivery_mode = PV_DELIVERY_MODE;
kvm_irq_delivery_to_apic(kvm, 0, &lapic_irq );
}
kvm/lapic.c
==========
_apic_accept_irq()
{
...
case APIC_DM_REMRD:
result = 1;
vcpu->pv_unhalted = 1
smp_mb();
kvm_make_request(KVM_REQ_EVENT, vcpu);
kvm_vcpu_kick(vcpu);
break;
...
}
here using PV_DELIVERY_MODE = APIC_DM_REMRD, which was unused.
OR
1) Are you asking to remove vcpu->pv_unhalted flag and replace with an irq?
2) are you talking about some reserved fields in struct local_apic instead
of APIC_DM_REMRD what I have used above?
[ Honestly, arch/x86/include/asm/apicdef.h had too much of info to
digest :( ]
3) I am not sure about: disallow guest to trigger it through apic
interface part also.(mean howto?)
4) And one more question, So you think it takes care of migration part
(in case we are removing pv_unhalted flag)?
It would be helpful if you can give little more explanation/ pointer to
Documentation.
Ingo is keen to see whole ticketlock/Xen/KVM patch in one go.
and anyhow since this does not cause any ABI change, I hope you don't
mind if
I do only the vcpu->pv_unhalted change you suggested now [ having
pv_unhalted reset in vcpu_run if
you meant something else than code I have above ], so that whole series
get fair amount time for testing.
^ permalink raw reply
* Re: [Xen-devel] [PATCH] drivers/video/xen-fbfront.c: add missing cleanup code
From: Konrad Rzeszutek Wilk @ 2012-04-26 21:42 UTC (permalink / raw)
To: Julia Lawall
Cc: Jeremy Fitzhardinge, xen-devel, Florian Tobias Schandinat,
kernel-janitors, linux-kernel, virtualization, linux-fbdev
In-Reply-To: <1335088660-13219-1-git-send-email-Julia.Lawall@lip6.fr>
On Sun, Apr 22, 2012 at 11:57:40AM +0200, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> The operations in the subsequent error-handling code appear to be also
> useful here.
How about doing it this way?
Florian, are you OK me carrying this patch in my tree for Linus or would
you prefer to do it?
commit a833fb9973b47cb30d1086e73f20b62a425bcd85
Author: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Sun Apr 22 11:57:40 2012 +0200
drivers/video/xen-fbfront.c: add missing cleanup code
The operations in the subsequent error-handling code appear to be also
useful here.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
[v1: Collapse some of the error handling functions]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index cb4529c..aa42160 100644
--- a/drivers/video/xen-fbfront.c
+++ b/drivers/video/xen-fbfront.c
@@ -458,26 +458,31 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
xenfb_init_shared_page(info, fb_info);
ret = xenfb_connect_backend(dev, info);
- if (ret < 0)
- goto error;
+ if (ret < 0) {
+ xenbus_dev_fatal(dev, ret, "xenfb_connect_backend");
+ goto error_fb;
+ }
ret = register_framebuffer(fb_info);
if (ret) {
- fb_deferred_io_cleanup(fb_info);
- fb_dealloc_cmap(&fb_info->cmap);
- framebuffer_release(fb_info);
xenbus_dev_fatal(dev, ret, "register_framebuffer");
- goto error;
+ goto error_fb;
}
info->fb_info = fb_info;
xenfb_make_preferred_console();
return 0;
- error_nomem:
- ret = -ENOMEM;
- xenbus_dev_fatal(dev, ret, "allocating device memory");
- error:
+error_fb:
+ fb_deferred_io_cleanup(fb_info);
+ fb_dealloc_cmap(&fb_info->cmap);
+ framebuffer_release(fb_info);
+error_nomem:
+ if (!ret) {
+ ret = -ENOMEM;
+ xenbus_dev_fatal(dev, ret, "allocating device memory");
+ }
+error:
xenfb_remove(dev);
return ret;
}
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> Not tested.
>
> drivers/video/xen-fbfront.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
> index cb4529c..b0bd59c 100644
> --- a/drivers/video/xen-fbfront.c
> +++ b/drivers/video/xen-fbfront.c
> @@ -458,8 +458,13 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
> xenfb_init_shared_page(info, fb_info);
>
> ret = xenfb_connect_backend(dev, info);
> - if (ret < 0)
> + if (ret < 0) {
> + fb_deferred_io_cleanup(fb_info);
> + fb_dealloc_cmap(&fb_info->cmap);
> + framebuffer_release(fb_info);
> + xenbus_dev_fatal(dev, ret, "xenfb_connect_backend");
> goto error;
> + }
>
> ret = register_framebuffer(fb_info);
> if (ret) {
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply related
* Re: [PATCH 2/3] virtio: balloon: let host know of updated balloon size before module removal
From: Michael S. Tsirkin @ 2012-04-26 20:50 UTC (permalink / raw)
To: Amit Shah; +Cc: Virtualization List
In-Reply-To: <22d281728c195b275e909be36535d9e14cefff27.1335467427.git.amit.shah@redhat.com>
On Fri, Apr 27, 2012 at 12:45:56AM +0530, Amit Shah wrote:
> When the balloon module is removed, we deflate the balloon, reclaiming
> all the pages that were given to the host. However, we don't update the
> config values for the new balloon size, resulting in the host showing
> outdated balloon values.
>
> The size update is done after each leak and fill operation, only the
> module removal case was left out.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
applied,
thanks
the rest are 3.5 material imo
> ---
> drivers/virtio/virtio_balloon.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 6921326..04baad6 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -390,6 +390,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
> /* There might be pages left in the balloon: free them. */
> while (vb->num_pages)
> leak_balloon(vb, vb->num_pages);
> + update_balloon_size(vb);
>
> /* Now we reset the device so we can clean up the queues. */
> vdev->config->reset(vdev);
> --
> 1.7.7.6
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox