[Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Gabriel L. Somlo]

On Mon, May 05, 2014 at 01:42:00PM +0200, Alexander Graf wrote:
> From what I understand about the compat bits (which is little) this
> makes a lot of sense. And the version is a proper property now,
> which is great :).
> 
> Acked-by: Alexander Graf <agraf@suse.de>

Thanks! Here's a new version with only 8 bits dedicated to the
"implementation version", as specified in the documentation for
the "apic version register".

Cheers,
  Gabriel

Gabriel L. Somlo (2):
  pc: add compat_props placeholder for 2.0 machine type
  apic: use emulated lapic version 0x14 on pc machines >= 2.1

 hw/i386/pc_piix.c               |  4 ++++
 hw/i386/pc_q35.c                |  4 ++++
 hw/intc/apic.c                  |  2 +-
 hw/intc/apic_common.c           |  1 +
 include/hw/i386/apic_internal.h |  1 +
 include/hw/i386/pc.h            | 12 ++++++++++++
 6 files changed, 23 insertions(+), 1 deletion(-)

-- 
1.9.0

[Qemu-devel] [v5 PATCH 1/2] pc: add compat_props placeholder for 2.0 machine type

[Gabriel L. Somlo]

Add the "boilerplate" necessary for subsequent patches to
simply drop in compat_props for pc machines 2.0 and older.

This patch contains no functional changes.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/pc_piix.c    | 4 ++++
 hw/i386/pc_q35.c     | 4 ++++
 include/hw/i386/pc.h | 9 +++++++++
 3 files changed, 17 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5b3594b..7c672d7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -415,6 +415,10 @@ static QEMUMachine pc_i440fx_machine_v2_0 = {
     PC_I440FX_2_0_MACHINE_OPTIONS,
     .name = "pc-i440fx-2.0",
     .init = pc_init_pci_2_0,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_2_0,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_1_7_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 5b48231..1fe2213 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -328,6 +328,10 @@ static QEMUMachine pc_q35_machine_v2_0 = {
     PC_Q35_2_0_MACHINE_OPTIONS,
     .name = "pc-q35-2.0",
     .init = pc_q35_init_2_0,
+    .compat_props = (GlobalProperty[]) {
+        PC_Q35_COMPAT_2_0,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_1_7_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9f26e14..0ade0f1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -242,8 +242,12 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
+#define PC_Q35_COMPAT_2_0 \
+        PC_COMPAT_2_0
+
 #define PC_Q35_COMPAT_1_7 \
         PC_COMPAT_1_7, \
+        PC_Q35_COMPAT_2_0, \
         {\
             .driver   = "hpet",\
             .property = HPET_INTCAP,\
@@ -262,7 +266,12 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         PC_COMPAT_1_4, \
         PC_Q35_COMPAT_1_5
 
+#define PC_COMPAT_2_0 \
+        {\
+        }
+
 #define PC_COMPAT_1_7 \
+        PC_COMPAT_2_0, \
         {\
             .driver   = TYPE_USB_DEVICE,\
             .property = "msos-desc",\
-- 
1.9.0

[Qemu-devel] [v5 PATCH 2/2] apic: use emulated lapic version 0x14 on pc machines >= 2.1

[Gabriel L. Somlo]

Add "version" property to local apic, and have it default to
0x14 for pc machines starting at 2.1. For compatibility with
previous releases, pc machines up to 2.0 will have their local
apic version set to 0x11.

Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
---
 hw/intc/apic.c                  | 2 +-
 hw/intc/apic_common.c           | 1 +
 include/hw/i386/apic_internal.h | 1 +
 include/hw/i386/pc.h            | 3 +++
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 2f40cba..ef19e55 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -675,7 +675,7 @@ static uint32_t apic_mem_readl(void *opaque, hwaddr addr)
         val = s->id << 24;
         break;
     case 0x03: /* version */
-        val = 0x11 | ((APIC_LVT_NB - 1) << 16); /* version 0x11 */
+        val = s->version | ((APIC_LVT_NB - 1) << 16);
         break;
     case 0x08:
         apic_sync_vapic(s, SYNC_FROM_VAPIC);
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 7ecce2d..7137653 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -380,6 +380,7 @@ static const VMStateDescription vmstate_apic_common = {
 
 static Property apic_properties_common[] = {
     DEFINE_PROP_UINT8("id", APICCommonState, id, -1),
+    DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14),
     DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
                     true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 70542a6..83e2a42 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -98,6 +98,7 @@ struct APICCommonState {
     X86CPU *cpu;
     uint32_t apicbase;
     uint8_t id;
+    uint8_t version;
     uint8_t arb_id;
     uint8_t tpr;
     uint32_t spurious_vec;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 0ade0f1..32a7687 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -268,6 +268,9 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_0 \
         {\
+            .driver   = "apic",\
+            .property = "version",\
+            .value    = stringify(0x11),\
         }
 
 #define PC_COMPAT_1_7 \
-- 
1.9.0

Re: [Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Michael S. Tsirkin]

On Mon, May 05, 2014 at 10:52:49AM -0400, Gabriel L. Somlo wrote:
> On Mon, May 05, 2014 at 01:42:00PM +0200, Alexander Graf wrote:
> > From what I understand about the compat bits (which is little) this
> > makes a lot of sense. And the version is a proper property now,
> > which is great :).
> > 
> > Acked-by: Alexander Graf <agraf@suse.de>
> 
> Thanks! Here's a new version with only 8 bits dedicated to the
> "implementation version", as specified in the documentation for
> the "apic version register".
> 
> Cheers,
>   Gabriel


Acked-by: Michael S. Tsirkin <mst@redhat.com>

I'll pick this up unless there are objections.

> Gabriel L. Somlo (2):
>   pc: add compat_props placeholder for 2.0 machine type
>   apic: use emulated lapic version 0x14 on pc machines >= 2.1
> 
>  hw/i386/pc_piix.c               |  4 ++++
>  hw/i386/pc_q35.c                |  4 ++++
>  hw/intc/apic.c                  |  2 +-
>  hw/intc/apic_common.c           |  1 +
>  include/hw/i386/apic_internal.h |  1 +
>  include/hw/i386/pc.h            | 12 ++++++++++++
>  6 files changed, 23 insertions(+), 1 deletion(-)
> 
> -- 
> 1.9.0

Re: [Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Andreas Färber]

Am 05.05.2014 17:51, schrieb Michael S. Tsirkin:
> On Mon, May 05, 2014 at 10:52:49AM -0400, Gabriel L. Somlo wrote:
>> On Mon, May 05, 2014 at 01:42:00PM +0200, Alexander Graf wrote:
>>> From what I understand about the compat bits (which is little) this
>>> makes a lot of sense. And the version is a proper property now,
>>> which is great :).
>>>
>>> Acked-by: Alexander Graf <agraf@suse.de>
>>
>> Thanks! Here's a new version with only 8 bits dedicated to the
>> "implementation version", as specified in the documentation for
>> the "apic version register".
>>
>> Cheers,
>>   Gabriel
> 
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I'll pick this up unless there are objections.

Objection! .compat_props are being added to the current pc and q35
machines rather than introducing pc-i440fx-2.1 and pc-q35-2.1 machines
and adding the compat_props for the 2.0 versions only.

If 2.1 machines without .compat_props get added and the alias is moved
from 2.0 to 2.1, it'll be fine.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Gabriel L. Somlo]

Andreas,

On Mon, May 05, 2014 at 06:00:22PM +0200, Andreas F?rber wrote:
> Objection! .compat_props are being added to the current pc and q35
> machines rather than introducing pc-i440fx-2.1 and pc-q35-2.1 machines
> and adding the compat_props for the 2.0 versions only.
> 
> If 2.1 machines without .compat_props get added and the alias is moved
> from 2.0 to 2.1, it'll be fine.

Patch 1/2 (add compat_props placeholder for 2.0) is written to go in
on top of this patch:

http://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg00476.html

which is the one adding the 2.1 machine type and moving the alias.


The pair of apic patches under discussion here modifies the (by now
legacy) 2.0 machine type.

Does that clear it up, or am I still misunderstanding something ?

Thanks,
--Gabriel

Re: [Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Andreas Färber]

Am 05.05.2014 18:21, schrieb Gabriel L. Somlo:
> Andreas,
> 
> On Mon, May 05, 2014 at 06:00:22PM +0200, Andreas F?rber wrote:
>> Objection! .compat_props are being added to the current pc and q35
>> machines rather than introducing pc-i440fx-2.1 and pc-q35-2.1 machines
>> and adding the compat_props for the 2.0 versions only.
>>
>> If 2.1 machines without .compat_props get added and the alias is moved
>> from 2.0 to 2.1, it'll be fine.
> 
> Patch 1/2 (add compat_props placeholder for 2.0) is written to go in
> on top of this patch:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg00476.html
> 
> which is the one adding the 2.1 machine type and moving the alias.
> 
> 
> The pair of apic patches under discussion here modifies the (by now
> legacy) 2.0 machine type.
> 
> Does that clear it up, or am I still misunderstanding something ?

Yes, with that patch it's okay, you just forgot to mention that
dependency in your cover letter - also a change log from v1 is missing.
Instead of quoting Alex in the cover letter, you should've placed his
Acked-by before your Signed-off-by in the patches he ack'ed - unless you
did major changes there (e.g., uint8_t), in which case it shouldn't be
in the cover letter either. And please use [PATCH v5 n/m] as canonical
ordering. :)

I trust that you have tested and other reviewers have considered no cast
to be necessary for left-hand s->version in the expression now that it's
uint8_t rather than uint32_t? Then,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Gabriel L. Somlo]

On Mon, May 05, 2014 at 07:38:58PM +0200, Andreas F?rber wrote:
> Yes, with that patch it's okay, you just forgot to mention that
> dependency in your cover letter - also a change log from v1 is missing.
> Instead of quoting Alex in the cover letter, you should've placed his
> Acked-by before your Signed-off-by in the patches he ack'ed - unless you
> did major changes there (e.g., uint8_t), in which case it shouldn't be
> in the cover letter either. And please use [PATCH v5 n/m] as canonical
> ordering. :)

You're right; the dependency was mentioned in the v4 cover letter, but
in retrospect it makes perfect sense I should have kept appending to that
content instead of using it as a place to reply to the last person who
commented on the previous version :)

Re. all that stuff you said about how to handle acked-by and
reviewed-by replies, is there a good spot where that process is
documented ? I noticed you all have a protocol in place for dealing
with that, but this is the first time I had a chance to screw it
up myself :) Googling around, I found this:

https://www.kernel.org/doc/Documentation/SubmittingPatches

Does QEMU have its own, or is this what I need for future reference ?

> I trust that you have tested and other reviewers have considered no cast
> to be necessary for left-hand s->version in the expression now that it's
> uint8_t rather than uint32_t? Then,

I get no compiler warnings, and adjacent case branches also assign
other 8-bit values to the 32-bit "val" variable w/o a cast, so I think
we're OK.

> 
> Reviewed-by: Andreas F?rber <afaerber@suse.de>

Thanks much,
--Gabriel

Re: [Qemu-devel] [v5 PATCH 0/2] apic: bump emulated lapic version to 0x14

[Alexander Graf]

On 05.05.14 20:08, Gabriel L. Somlo wrote:
> On Mon, May 05, 2014 at 07:38:58PM +0200, Andreas F?rber wrote:
>> Yes, with that patch it's okay, you just forgot to mention that
>> dependency in your cover letter - also a change log from v1 is missing.
>> Instead of quoting Alex in the cover letter, you should've placed his
>> Acked-by before your Signed-off-by in the patches he ack'ed - unless you
>> did major changes there (e.g., uint8_t), in which case it shouldn't be
>> in the cover letter either. And please use [PATCH v5 n/m] as canonical
>> ordering. :)
> You're right; the dependency was mentioned in the v4 cover letter, but
> in retrospect it makes perfect sense I should have kept appending to that
> content instead of using it as a place to reply to the last person who
> commented on the previous version :)
>
> Re. all that stuff you said about how to handle acked-by and
> reviewed-by replies, is there a good spot where that process is
> documented ? I noticed you all have a protocol in place for dealing
> with that, but this is the first time I had a chance to screw it
> up myself :) Googling around, I found this:
>
> https://www.kernel.org/doc/Documentation/SubmittingPatches
>
> Does QEMU have its own, or is this what I need for future reference ?

We have some help on top at

   http://wiki.qemu.org/Contribute/SubmitAPatch

but if you simply stick to what Linux does you're mostly fine in QEMU 
land as well :)


Alex