[Qemu-devel] [PATCH 0/2] pc: fixes broken compat logic

[Qemu-devel] [PATCH 0/2] pc: fixes broken compat logic

[Igor Mammedov]

fixes mistakes made by:

 targte-i386: Add virtual L3 cache support
 https://www.mail-archive.com/qemu-devel@nongnu.org/msg395418.html

Igor Mammedov (2):
  pc: fix regression introduced by adding 2.8 machine
  target-i386: turn off CPU.l3-cache only for 2.7 and older machine
    types

 include/hw/i386/pc.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] pc: fix regression introduced by adding 2.8 machine

[Igor Mammedov]

commit (a4d3c834 pc: Add 2.8 machine) didn't ammend
PC_COMPAT_2_6 to include PC_COMPAT_2_7 which results in

    {\
        .driver   = "virtio-pci",\
        .property = "page-per-vq",\
        .value    = "on",\
    },

and

    {\
        .driver   = TYPE_X86_CPU,\
        .property = "l3-cache",\
        .value    = "off",\
    },

being ignored and as result change PC_COMPAT_2_6 behaviour.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/pc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ebba151..d5654ab 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -381,6 +381,7 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
     HW_COMPAT_2_7
 
 #define PC_COMPAT_2_6 \
+    PC_COMPAT_2_7 \
     HW_COMPAT_2_6 \
     {\
         .driver   = "fw_cfg_io",\
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] pc: fix regression introduced by adding 2.8 machine

[Marcel Apfelbaum]

On 09/14/2016 01:01 PM, Igor Mammedov wrote:
> commit (a4d3c834 pc: Add 2.8 machine) didn't ammend
> PC_COMPAT_2_6 to include PC_COMPAT_2_7 which results in
>
>     {\
>         .driver   = "virtio-pci",\
>         .property = "page-per-vq",\
>         .value    = "on",\
>     },
>
> and
>
>     {\
>         .driver   = TYPE_X86_CPU,\
>         .property = "l3-cache",\
>         .value    = "off",\
>     },
>
> being ignored and as result change PC_COMPAT_2_6 behaviour.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/i386/pc.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ebba151..d5654ab 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -381,6 +381,7 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>      HW_COMPAT_2_7
>
>  #define PC_COMPAT_2_6 \
> +    PC_COMPAT_2_7 \
>      HW_COMPAT_2_6 \
>      {\
>          .driver   = "fw_cfg_io",\
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 1/2] pc: fix regression introduced by adding 2.8 machine

[Eduardo Habkost]

On Wed, Sep 14, 2016 at 12:01:49PM +0200, Igor Mammedov wrote:
> commit (a4d3c834 pc: Add 2.8 machine) didn't ammend
> PC_COMPAT_2_6 to include PC_COMPAT_2_7 which results in
> 
>     {\
>         .driver   = "virtio-pci",\
>         .property = "page-per-vq",\
>         .value    = "on",\
>     },
> 
> and
> 
>     {\
>         .driver   = TYPE_X86_CPU,\
>         .property = "l3-cache",\
>         .value    = "off",\
>     },
> 
> being ignored and as result change PC_COMPAT_2_6 behaviour.
> 
[...]
>  #define PC_COMPAT_2_6 \
> +    PC_COMPAT_2_7 \
>      HW_COMPAT_2_6 \
>      {\
>          .driver   = "fw_cfg_io",\

Isn't this supposed to be unnecessary since commit bacc344c
("machine: add properties to compat_props incrementaly")?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] pc: fix regression introduced by adding 2.8 machine

[Michael S. Tsirkin]

On Thu, Sep 15, 2016 at 11:19:09AM -0300, Eduardo Habkost wrote:
> On Wed, Sep 14, 2016 at 12:01:49PM +0200, Igor Mammedov wrote:
> > commit (a4d3c834 pc: Add 2.8 machine) didn't ammend
> > PC_COMPAT_2_6 to include PC_COMPAT_2_7 which results in
> > 
> >     {\
> >         .driver   = "virtio-pci",\
> >         .property = "page-per-vq",\
> >         .value    = "on",\
> >     },
> > 
> > and
> > 
> >     {\
> >         .driver   = TYPE_X86_CPU,\
> >         .property = "l3-cache",\
> >         .value    = "off",\
> >     },
> > 
> > being ignored and as result change PC_COMPAT_2_6 behaviour.
> > 
> [...]
> >  #define PC_COMPAT_2_6 \
> > +    PC_COMPAT_2_7 \
> >      HW_COMPAT_2_6 \
> >      {\
> >          .driver   = "fw_cfg_io",\
> 
> Isn't this supposed to be unnecessary since commit bacc344c
> ("machine: add properties to compat_props incrementaly")?
> 
> -- 
> Eduardo


Indeed, I tested page-per-vq and it seems on for 2.6.
Igor, Marcel, could you comment pls?


-- 
MST

Re: [Qemu-devel] [PATCH 1/2] pc: fix regression introduced by adding 2.8 machine

[Igor Mammedov]

On Thu, 15 Sep 2016 18:18:46 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 15, 2016 at 11:19:09AM -0300, Eduardo Habkost wrote:
> > On Wed, Sep 14, 2016 at 12:01:49PM +0200, Igor Mammedov wrote:  
> > > commit (a4d3c834 pc: Add 2.8 machine) didn't ammend
> > > PC_COMPAT_2_6 to include PC_COMPAT_2_7 which results in
> > > 
> > >     {\
> > >         .driver   = "virtio-pci",\
> > >         .property = "page-per-vq",\
> > >         .value    = "on",\
> > >     },
> > > 
> > > and
> > > 
> > >     {\
> > >         .driver   = TYPE_X86_CPU,\
> > >         .property = "l3-cache",\
> > >         .value    = "off",\
> > >     },
> > > 
> > > being ignored and as result change PC_COMPAT_2_6 behaviour.
> > >   
> > [...]  
> > >  #define PC_COMPAT_2_6 \
> > > +    PC_COMPAT_2_7 \
> > >      HW_COMPAT_2_6 \
> > >      {\
> > >          .driver   = "fw_cfg_io",\  
> > 
> > Isn't this supposed to be unnecessary since commit bacc344c
> > ("machine: add properties to compat_props incrementaly")?
> > 
> > -- 
> > Eduardo  
> 
> 
> Indeed, I tested page-per-vq and it seems on for 2.6.
> Igor, Marcel, could you comment pls?

You are right, I think inconsistency in

#define PC_COMPAT_2_5 \                                                          
    PC_COMPAT_2_6 \                                                              
    HW_COMPAT_2_5  
 
and

#define PC_COMPAT_2_7 \                                                          
    PC_COMPAT_2_8 \                                                              
    HW_COMPAT_2_7

made me think that we still need it.

I'd better clean-up PC_COMPAT_2_5 and PC_COMPAT_2_7

[Qemu-devel] [PATCH 2/2] target-i386: turn off CPU.l3-cache only for 2.7 and older machine types

[Igor Mammedov]

commit (14c985cff target-i386: present virtual L3 cache info for vcpus)
misplaced compat property putting it in new 2.8 machine type
which would effectively to disable feature until 2.9 is released.
Intent of commit probably should be to disable feature for 2.7
and older while allowing not yet released 2.8 to have feature
enabled by default.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/pc.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d5654ab..1c5fd08 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -369,17 +369,16 @@ int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_8 \
+
+#define PC_COMPAT_2_7 \
+    PC_COMPAT_2_8 \
+    HW_COMPAT_2_7 \
     {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\
     },
 
-
-#define PC_COMPAT_2_7 \
-    PC_COMPAT_2_8 \
-    HW_COMPAT_2_7
-
 #define PC_COMPAT_2_6 \
     PC_COMPAT_2_7 \
     HW_COMPAT_2_6 \
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] target-i386: turn off CPU.l3-cache only for 2.7 and older machine types

[Marcel Apfelbaum]

On 09/14/2016 01:01 PM, Igor Mammedov wrote:
> commit (14c985cff target-i386: present virtual L3 cache info for vcpus)
> misplaced compat property putting it in new 2.8 machine type
> which would effectively to disable feature until 2.9 is released.
> Intent of commit probably should be to disable feature for 2.7
> and older while allowing not yet released 2.8 to have feature
> enabled by default.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/i386/pc.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d5654ab..1c5fd08 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -369,17 +369,16 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>
>  #define PC_COMPAT_2_8 \
> +
> +#define PC_COMPAT_2_7 \
> +    PC_COMPAT_2_8 \
> +    HW_COMPAT_2_7 \
>      {\
>          .driver   = TYPE_X86_CPU,\
>          .property = "l3-cache",\
>          .value    = "off",\
>      },
>
> -
> -#define PC_COMPAT_2_7 \
> -    PC_COMPAT_2_8 \
> -    HW_COMPAT_2_7
> -
>  #define PC_COMPAT_2_6 \
>      PC_COMPAT_2_7 \
>      HW_COMPAT_2_6 \
>

Re: [Qemu-devel] [PATCH 2/2] target-i386: turn off CPU.l3-cache only for 2.7 and older machine types

[Eduardo Habkost]

On Wed, Sep 14, 2016 at 12:01:50PM +0200, Igor Mammedov wrote:
> commit (14c985cff target-i386: present virtual L3 cache info for vcpus)
> misplaced compat property putting it in new 2.8 machine type
> which would effectively to disable feature until 2.9 is released.
> Intent of commit probably should be to disable feature for 2.7
> and older while allowing not yet released 2.8 to have feature
> enabled by default.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/i386/pc.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d5654ab..1c5fd08 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -369,17 +369,16 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_8 \
> +
> +#define PC_COMPAT_2_7 \
> +    PC_COMPAT_2_8 \

Same as patch 1/2: this doesn't seem to be necessary since commit
bacc344c548ce165a0001276ece56ee4b0bddae3.

> +    HW_COMPAT_2_7 \
>      {\
>          .driver   = TYPE_X86_CPU,\
>          .property = "l3-cache",\
>          .value    = "off",\
>      },
>  
> -
> -#define PC_COMPAT_2_7 \
> -    PC_COMPAT_2_8 \
> -    HW_COMPAT_2_7
> -
>  #define PC_COMPAT_2_6 \
>      PC_COMPAT_2_7 \
>      HW_COMPAT_2_6 \
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] target-i386: turn off CPU.l3-cache only for 2.7 and older machine types

[Igor Mammedov]

On Thu, 15 Sep 2016 11:23:47 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Sep 14, 2016 at 12:01:50PM +0200, Igor Mammedov wrote:
> > commit (14c985cff target-i386: present virtual L3 cache info for vcpus)
> > misplaced compat property putting it in new 2.8 machine type
> > which would effectively to disable feature until 2.9 is released.
> > Intent of commit probably should be to disable feature for 2.7
> > and older while allowing not yet released 2.8 to have feature
> > enabled by default.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/i386/pc.h | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index d5654ab..1c5fd08 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -369,17 +369,16 @@ int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  
> >  #define PC_COMPAT_2_8 \
> > +
> > +#define PC_COMPAT_2_7 \
> > +    PC_COMPAT_2_8 \  
> 
> Same as patch 1/2: this doesn't seem to be necessary since commit
> bacc344c548ce165a0001276ece56ee4b0bddae3.

"l3-cache" is off for 2.8 regardless of bacc344c while it should be off
only for 2.7 and older.

Anyway, I'll respin because of 1/2 should be rewritten.

> 
> > +    HW_COMPAT_2_7 \
> >      {\
> >          .driver   = TYPE_X86_CPU,\
> >          .property = "l3-cache",\
> >          .value    = "off",\
> >      },
> >  
> > -
> > -#define PC_COMPAT_2_7 \
> > -    PC_COMPAT_2_8 \
> > -    HW_COMPAT_2_7
> > -
> >  #define PC_COMPAT_2_6 \
> >      PC_COMPAT_2_7 \
> >      HW_COMPAT_2_6 \
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH 2/2] target-i386: turn off CPU.l3-cache only for 2.7 and older machine types

[Eduardo Habkost]

On Fri, Sep 16, 2016 at 10:08:30AM +0200, Igor Mammedov wrote:
> On Thu, 15 Sep 2016 11:23:47 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Sep 14, 2016 at 12:01:50PM +0200, Igor Mammedov wrote:
> > > commit (14c985cff target-i386: present virtual L3 cache info for vcpus)
> > > misplaced compat property putting it in new 2.8 machine type
> > > which would effectively to disable feature until 2.9 is released.
> > > Intent of commit probably should be to disable feature for 2.7
> > > and older while allowing not yet released 2.8 to have feature
> > > enabled by default.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  include/hw/i386/pc.h | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index d5654ab..1c5fd08 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -369,17 +369,16 @@ int e820_get_num_entries(void);
> > >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> > >  
> > >  #define PC_COMPAT_2_8 \
> > > +
> > > +#define PC_COMPAT_2_7 \
> > > +    PC_COMPAT_2_8 \  
> > 
> > Same as patch 1/2: this doesn't seem to be necessary since commit
> > bacc344c548ce165a0001276ece56ee4b0bddae3.
> 
> "l3-cache" is off for 2.8 regardless of bacc344c while it should be off
> only for 2.7 and older.
> 
> Anyway, I'll respin because of 1/2 should be rewritten.

Yes, l3-cache on PC_COMPAT_2_7 is necessary. I was only talking
about the PC_COMPAT_2_8 line above, because it looked like a
newly added line (but now I see it was just the line from patch
1/2 being moved, sorry for the confusion).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] pc: fixes broken compat logic

[Longpeng (Mike)]

Hi Igor,

On 2016/9/14 18:01, Igor Mammedov wrote:

> fixes mistakes made by:
> 
>  targte-i386: Add virtual L3 cache support
>  https://www.mail-archive.com/qemu-devel@nongnu.org/msg395418.html
> 
> Igor Mammedov (2):
>   pc: fix regression introduced by adding 2.8 machine
>   target-i386: turn off CPU.l3-cache only for 2.7 and older machine
>     types
> 
>  include/hw/i386/pc.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

You're right, thank you for correcting my mistakes :)

-- 
Regards,
Longpeng(Mike)