[Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Michael S. Tsirkin]

kvm_add_routing_entry makes an attempt to
zero-initialize any new routing entry.
However, it fails to initialize padding
within the u field of the structure
kvm_irq_routing_entry.

Other functions like kvm_irqchip_update_msi_route
also fail to initialize the padding field in
kvm_irq_routing_entry.

While mostly harmless, this would prevent us from
reusing these fields for something useful in
the future.

It's better to just make sure all input is initialized.

Once it is, we can also drop complex field by field assignment and just
do the simple *a = *b to update a route entry.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm-all.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 405480e..f119ce1 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
     }
     n = s->irq_routes->nr++;
     new = &s->irq_routes->entries[n];
-    memset(new, 0, sizeof(*new));
-    new->gsi = entry->gsi;
-    new->type = entry->type;
-    new->flags = entry->flags;
-    new->u = entry->u;
+
+    *new = *entry;
 
     set_gsi(s, entry->gsi);
 
@@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
             continue;
         }
 
-        entry->type = new_entry->type;
-        entry->flags = new_entry->flags;
-        entry->u = new_entry->u;
+        *entry = *new_entry;
 
         kvm_irqchip_commit_routes(s);
 
@@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
 
 void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
 {
-    struct kvm_irq_routing_entry e;
+    struct kvm_irq_routing_entry e = {};
 
     assert(pin < s->gsi_count);
 
@@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
             return virq;
         }
 
-        route = g_malloc(sizeof(KVMMSIRoute));
+        route = g_malloc0(sizeof(KVMMSIRoute));
         route->kroute.gsi = virq;
         route->kroute.type = KVM_IRQ_ROUTING_MSI;
         route->kroute.flags = 0;
@@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
 
 int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
 {
-    struct kvm_irq_routing_entry kroute;
+    struct kvm_irq_routing_entry kroute = {};
     int virq;
 
     if (!kvm_gsi_routing_enabled()) {
@@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
 
 int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
 {
-    struct kvm_irq_routing_entry kroute;
+    struct kvm_irq_routing_entry kroute = {};
 
     if (!kvm_irqchip_in_kernel()) {
         return -ENOSYS;
-- 
MST

[Qemu-devel] [PATCH 2/2] kvm: skip system call when msi route is unchanged

[Michael S. Tsirkin]

Some guests do a large number of mask/unmask
calls which currently trigger expensive route update
system calls.
Detect that route in unchanged and skip the system call.

Reported-by: "Zhanghaoyu (A)" <haoyu.zhang@huawei.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm-all.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kvm-all.c b/kvm-all.c
index f119ce1..891722b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1026,6 +1026,10 @@ static int kvm_update_routing_entry(KVMState *s,
             continue;
         }
 
+        if(!memcmp(entry, new_entry, sizeof *entry)) {
+            return 0;
+        }
+
         *entry = *new_entry;
 
         kvm_irqchip_commit_routes(s);
-- 
MST

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Gleb Natapov]

On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> kvm_add_routing_entry makes an attempt to
> zero-initialize any new routing entry.
> However, it fails to initialize padding
> within the u field of the structure
> kvm_irq_routing_entry.
> 
> Other functions like kvm_irqchip_update_msi_route
> also fail to initialize the padding field in
> kvm_irq_routing_entry.
> 
> While mostly harmless, this would prevent us from
> reusing these fields for something useful in
> the future.
> 
The fact that kernel does not check them for zero value is what will
prevents us from doing so.

> It's better to just make sure all input is initialized.
> 
> Once it is, we can also drop complex field by field assignment and just
> do the simple *a = *b to update a route entry.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  kvm-all.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 405480e..f119ce1 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
>      }
>      n = s->irq_routes->nr++;
>      new = &s->irq_routes->entries[n];
> -    memset(new, 0, sizeof(*new));
> -    new->gsi = entry->gsi;
> -    new->type = entry->type;
> -    new->flags = entry->flags;
> -    new->u = entry->u;
> +
> +    *new = *entry;
>  
>      set_gsi(s, entry->gsi);
>  
> @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
>              continue;
>          }
>  
> -        entry->type = new_entry->type;
> -        entry->flags = new_entry->flags;
> -        entry->u = new_entry->u;
> +        *entry = *new_entry;
>  
>          kvm_irqchip_commit_routes(s);
>  
> @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
>  
>  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
>  {
> -    struct kvm_irq_routing_entry e;
> +    struct kvm_irq_routing_entry e = {};
>  
>      assert(pin < s->gsi_count);
>  
> @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
>              return virq;
>          }
>  
> -        route = g_malloc(sizeof(KVMMSIRoute));
> +        route = g_malloc0(sizeof(KVMMSIRoute));
>          route->kroute.gsi = virq;
>          route->kroute.type = KVM_IRQ_ROUTING_MSI;
>          route->kroute.flags = 0;
> @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
>  
>  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
>  {
> -    struct kvm_irq_routing_entry kroute;
> +    struct kvm_irq_routing_entry kroute = {};
>      int virq;
>  
>      if (!kvm_gsi_routing_enabled()) {
> @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
>  
>  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
>  {
> -    struct kvm_irq_routing_entry kroute;
> +    struct kvm_irq_routing_entry kroute = {};
>  
>      if (!kvm_irqchip_in_kernel()) {
>          return -ENOSYS;
> -- 
> MST

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 04:00:20PM +0300, Gleb Natapov wrote:
> On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> > kvm_add_routing_entry makes an attempt to
> > zero-initialize any new routing entry.
> > However, it fails to initialize padding
> > within the u field of the structure
> > kvm_irq_routing_entry.
> > 
> > Other functions like kvm_irqchip_update_msi_route
> > also fail to initialize the padding field in
> > kvm_irq_routing_entry.
> > 
> > While mostly harmless, this would prevent us from
> > reusing these fields for something useful in
> > the future.
> > 
> The fact that kernel does not check them for zero value is what will
> prevents us from doing so.

Well we can not change kernel now (it would break userspace)
but we can start zeroing everything in userspace.

Also, checkers like coverity might get confused by this.

Finally, simpler assignment and comparison make it worth it,
don't they?

> > It's better to just make sure all input is initialized.
> > 
> > Once it is, we can also drop complex field by field assignment and just
> > do the simple *a = *b to update a route entry.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  kvm-all.c | 19 +++++++------------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 405480e..f119ce1 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
> >      }
> >      n = s->irq_routes->nr++;
> >      new = &s->irq_routes->entries[n];
> > -    memset(new, 0, sizeof(*new));
> > -    new->gsi = entry->gsi;
> > -    new->type = entry->type;
> > -    new->flags = entry->flags;
> > -    new->u = entry->u;
> > +
> > +    *new = *entry;
> >  
> >      set_gsi(s, entry->gsi);
> >  
> > @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
> >              continue;
> >          }
> >  
> > -        entry->type = new_entry->type;
> > -        entry->flags = new_entry->flags;
> > -        entry->u = new_entry->u;
> > +        *entry = *new_entry;
> >  
> >          kvm_irqchip_commit_routes(s);
> >  
> > @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
> >  
> >  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> >  {
> > -    struct kvm_irq_routing_entry e;
> > +    struct kvm_irq_routing_entry e = {};
> >  
> >      assert(pin < s->gsi_count);
> >  
> > @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> >              return virq;
> >          }
> >  
> > -        route = g_malloc(sizeof(KVMMSIRoute));
> > +        route = g_malloc0(sizeof(KVMMSIRoute));
> >          route->kroute.gsi = virq;
> >          route->kroute.type = KVM_IRQ_ROUTING_MSI;
> >          route->kroute.flags = 0;
> > @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> >  
> >  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> >  {
> > -    struct kvm_irq_routing_entry kroute;
> > +    struct kvm_irq_routing_entry kroute = {};
> >      int virq;
> >  
> >      if (!kvm_gsi_routing_enabled()) {
> > @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> >  
> >  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
> >  {
> > -    struct kvm_irq_routing_entry kroute;
> > +    struct kvm_irq_routing_entry kroute = {};
> >  
> >      if (!kvm_irqchip_in_kernel()) {
> >          return -ENOSYS;
> > -- 
> > MST
> 
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Gleb Natapov]

On Wed, Jun 05, 2013 at 05:02:07PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 05, 2013 at 04:00:20PM +0300, Gleb Natapov wrote:
> > On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> > > kvm_add_routing_entry makes an attempt to
> > > zero-initialize any new routing entry.
> > > However, it fails to initialize padding
> > > within the u field of the structure
> > > kvm_irq_routing_entry.
> > > 
> > > Other functions like kvm_irqchip_update_msi_route
> > > also fail to initialize the padding field in
> > > kvm_irq_routing_entry.
> > > 
> > > While mostly harmless, this would prevent us from
> > > reusing these fields for something useful in
> > > the future.
> > > 
> > The fact that kernel does not check them for zero value is what will
> > prevents us from doing so.
> 
> Well we can not change kernel now (it would break userspace)
> but we can start zeroing everything in userspace.
> 
> Also, checkers like coverity might get confused by this.
> 
> Finally, simpler assignment and comparison make it worth it,
> don't they?
> 
I am not at all against the patch! Just pointing out a mistake in the
commit message.

> > > It's better to just make sure all input is initialized.
> > > 
> > > Once it is, we can also drop complex field by field assignment and just
> > > do the simple *a = *b to update a route entry.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  kvm-all.c | 19 +++++++------------
> > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/kvm-all.c b/kvm-all.c
> > > index 405480e..f119ce1 100644
> > > --- a/kvm-all.c
> > > +++ b/kvm-all.c
> > > @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
> > >      }
> > >      n = s->irq_routes->nr++;
> > >      new = &s->irq_routes->entries[n];
> > > -    memset(new, 0, sizeof(*new));
> > > -    new->gsi = entry->gsi;
> > > -    new->type = entry->type;
> > > -    new->flags = entry->flags;
> > > -    new->u = entry->u;
> > > +
> > > +    *new = *entry;
> > >  
> > >      set_gsi(s, entry->gsi);
> > >  
> > > @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > >              continue;
> > >          }
> > >  
> > > -        entry->type = new_entry->type;
> > > -        entry->flags = new_entry->flags;
> > > -        entry->u = new_entry->u;
> > > +        *entry = *new_entry;
> > >  
> > >          kvm_irqchip_commit_routes(s);
> > >  
> > > @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > >  
> > >  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> > >  {
> > > -    struct kvm_irq_routing_entry e;
> > > +    struct kvm_irq_routing_entry e = {};
> > >  
> > >      assert(pin < s->gsi_count);
> > >  
> > > @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > >              return virq;
> > >          }
> > >  
> > > -        route = g_malloc(sizeof(KVMMSIRoute));
> > > +        route = g_malloc0(sizeof(KVMMSIRoute));
> > >          route->kroute.gsi = virq;
> > >          route->kroute.type = KVM_IRQ_ROUTING_MSI;
> > >          route->kroute.flags = 0;
> > > @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > >  
> > >  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > >  {
> > > -    struct kvm_irq_routing_entry kroute;
> > > +    struct kvm_irq_routing_entry kroute = {};
> > >      int virq;
> > >  
> > >      if (!kvm_gsi_routing_enabled()) {
> > > @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > >  
> > >  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
> > >  {
> > > -    struct kvm_irq_routing_entry kroute;
> > > +    struct kvm_irq_routing_entry kroute = {};
> > >  
> > >      if (!kvm_irqchip_in_kernel()) {
> > >          return -ENOSYS;
> > > -- 
> > > MST
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 05:04:55PM +0300, Gleb Natapov wrote:
> On Wed, Jun 05, 2013 at 05:02:07PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 05, 2013 at 04:00:20PM +0300, Gleb Natapov wrote:
> > > On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> > > > kvm_add_routing_entry makes an attempt to
> > > > zero-initialize any new routing entry.
> > > > However, it fails to initialize padding
> > > > within the u field of the structure
> > > > kvm_irq_routing_entry.
> > > > 
> > > > Other functions like kvm_irqchip_update_msi_route
> > > > also fail to initialize the padding field in
> > > > kvm_irq_routing_entry.
> > > > 
> > > > While mostly harmless, this would prevent us from
> > > > reusing these fields for something useful in
> > > > the future.
> > > > 
> > > The fact that kernel does not check them for zero value is what will
> > > prevents us from doing so.
> > 
> > Well we can not change kernel now (it would break userspace)
> > but we can start zeroing everything in userspace.
> > 
> > Also, checkers like coverity might get confused by this.
> > 
> > Finally, simpler assignment and comparison make it worth it,
> > don't they?
> > 
> I am not at all against the patch! Just pointing out a mistake in the
> commit message.

I think we can agree both userspace not initializing them and kernel not
checking it are mistakes?

Anyway ... could you commit this tweaking the commit
message in a way that you consider appropriate?
Or want me to repost?


> > > > It's better to just make sure all input is initialized.
> > > > 
> > > > Once it is, we can also drop complex field by field assignment and just
> > > > do the simple *a = *b to update a route entry.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  kvm-all.c | 19 +++++++------------
> > > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/kvm-all.c b/kvm-all.c
> > > > index 405480e..f119ce1 100644
> > > > --- a/kvm-all.c
> > > > +++ b/kvm-all.c
> > > > @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
> > > >      }
> > > >      n = s->irq_routes->nr++;
> > > >      new = &s->irq_routes->entries[n];
> > > > -    memset(new, 0, sizeof(*new));
> > > > -    new->gsi = entry->gsi;
> > > > -    new->type = entry->type;
> > > > -    new->flags = entry->flags;
> > > > -    new->u = entry->u;
> > > > +
> > > > +    *new = *entry;
> > > >  
> > > >      set_gsi(s, entry->gsi);
> > > >  
> > > > @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > > >              continue;
> > > >          }
> > > >  
> > > > -        entry->type = new_entry->type;
> > > > -        entry->flags = new_entry->flags;
> > > > -        entry->u = new_entry->u;
> > > > +        *entry = *new_entry;
> > > >  
> > > >          kvm_irqchip_commit_routes(s);
> > > >  
> > > > @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > > >  
> > > >  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> > > >  {
> > > > -    struct kvm_irq_routing_entry e;
> > > > +    struct kvm_irq_routing_entry e = {};
> > > >  
> > > >      assert(pin < s->gsi_count);
> > > >  
> > > > @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > > >              return virq;
> > > >          }
> > > >  
> > > > -        route = g_malloc(sizeof(KVMMSIRoute));
> > > > +        route = g_malloc0(sizeof(KVMMSIRoute));
> > > >          route->kroute.gsi = virq;
> > > >          route->kroute.type = KVM_IRQ_ROUTING_MSI;
> > > >          route->kroute.flags = 0;
> > > > @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > > >  
> > > >  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > > >  {
> > > > -    struct kvm_irq_routing_entry kroute;
> > > > +    struct kvm_irq_routing_entry kroute = {};
> > > >      int virq;
> > > >  
> > > >      if (!kvm_gsi_routing_enabled()) {
> > > > @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > > >  
> > > >  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
> > > >  {
> > > > -    struct kvm_irq_routing_entry kroute;
> > > > +    struct kvm_irq_routing_entry kroute = {};
> > > >  
> > > >      if (!kvm_irqchip_in_kernel()) {
> > > >          return -ENOSYS;
> > > > -- 
> > > > MST
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Gleb Natapov]

On Wed, Jun 05, 2013 at 05:11:44PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 05, 2013 at 05:04:55PM +0300, Gleb Natapov wrote:
> > On Wed, Jun 05, 2013 at 05:02:07PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 05, 2013 at 04:00:20PM +0300, Gleb Natapov wrote:
> > > > On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> > > > > kvm_add_routing_entry makes an attempt to
> > > > > zero-initialize any new routing entry.
> > > > > However, it fails to initialize padding
> > > > > within the u field of the structure
> > > > > kvm_irq_routing_entry.
> > > > > 
> > > > > Other functions like kvm_irqchip_update_msi_route
> > > > > also fail to initialize the padding field in
> > > > > kvm_irq_routing_entry.
> > > > > 
> > > > > While mostly harmless, this would prevent us from
> > > > > reusing these fields for something useful in
> > > > > the future.
> > > > > 
> > > > The fact that kernel does not check them for zero value is what will
> > > > prevents us from doing so.
> > > 
> > > Well we can not change kernel now (it would break userspace)
> > > but we can start zeroing everything in userspace.
> > > 
> > > Also, checkers like coverity might get confused by this.
> > > 
> > > Finally, simpler assignment and comparison make it worth it,
> > > don't they?
> > > 
> > I am not at all against the patch! Just pointing out a mistake in the
> > commit message.
> 
> I think we can agree both userspace not initializing them and kernel not
> checking it are mistakes?
> 
Mistake that cannot be fixed at this point.

> Anyway ... could you commit this tweaking the commit
> message in a way that you consider appropriate?
> Or want me to repost?
> 
No need to report.

> 
> > > > > It's better to just make sure all input is initialized.
> > > > > 
> > > > > Once it is, we can also drop complex field by field assignment and just
> > > > > do the simple *a = *b to update a route entry.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  kvm-all.c | 19 +++++++------------
> > > > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/kvm-all.c b/kvm-all.c
> > > > > index 405480e..f119ce1 100644
> > > > > --- a/kvm-all.c
> > > > > +++ b/kvm-all.c
> > > > > @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
> > > > >      }
> > > > >      n = s->irq_routes->nr++;
> > > > >      new = &s->irq_routes->entries[n];
> > > > > -    memset(new, 0, sizeof(*new));
> > > > > -    new->gsi = entry->gsi;
> > > > > -    new->type = entry->type;
> > > > > -    new->flags = entry->flags;
> > > > > -    new->u = entry->u;
> > > > > +
> > > > > +    *new = *entry;
> > > > >  
> > > > >      set_gsi(s, entry->gsi);
> > > > >  
> > > > > @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > > > >              continue;
> > > > >          }
> > > > >  
> > > > > -        entry->type = new_entry->type;
> > > > > -        entry->flags = new_entry->flags;
> > > > > -        entry->u = new_entry->u;
> > > > > +        *entry = *new_entry;
> > > > >  
> > > > >          kvm_irqchip_commit_routes(s);
> > > > >  
> > > > > @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > > > >  
> > > > >  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> > > > >  {
> > > > > -    struct kvm_irq_routing_entry e;
> > > > > +    struct kvm_irq_routing_entry e = {};
> > > > >  
> > > > >      assert(pin < s->gsi_count);
> > > > >  
> > > > > @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > > > >              return virq;
> > > > >          }
> > > > >  
> > > > > -        route = g_malloc(sizeof(KVMMSIRoute));
> > > > > +        route = g_malloc0(sizeof(KVMMSIRoute));
> > > > >          route->kroute.gsi = virq;
> > > > >          route->kroute.type = KVM_IRQ_ROUTING_MSI;
> > > > >          route->kroute.flags = 0;
> > > > > @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > > > >  
> > > > >  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > > > >  {
> > > > > -    struct kvm_irq_routing_entry kroute;
> > > > > +    struct kvm_irq_routing_entry kroute = {};
> > > > >      int virq;
> > > > >  
> > > > >      if (!kvm_gsi_routing_enabled()) {
> > > > > @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > > > >  
> > > > >  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
> > > > >  {
> > > > > -    struct kvm_irq_routing_entry kroute;
> > > > > +    struct kvm_irq_routing_entry kroute = {};
> > > > >  
> > > > >      if (!kvm_irqchip_in_kernel()) {
> > > > >          return -ENOSYS;
> > > > > -- 
> > > > > MST
> > > > 
> > > > --
> > > > 			Gleb.
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 05:12:32PM +0300, Gleb Natapov wrote:
> On Wed, Jun 05, 2013 at 05:11:44PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 05, 2013 at 05:04:55PM +0300, Gleb Natapov wrote:
> > > On Wed, Jun 05, 2013 at 05:02:07PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 05, 2013 at 04:00:20PM +0300, Gleb Natapov wrote:
> > > > > On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> > > > > > kvm_add_routing_entry makes an attempt to
> > > > > > zero-initialize any new routing entry.
> > > > > > However, it fails to initialize padding
> > > > > > within the u field of the structure
> > > > > > kvm_irq_routing_entry.
> > > > > > 
> > > > > > Other functions like kvm_irqchip_update_msi_route
> > > > > > also fail to initialize the padding field in
> > > > > > kvm_irq_routing_entry.
> > > > > > 
> > > > > > While mostly harmless, this would prevent us from
> > > > > > reusing these fields for something useful in
> > > > > > the future.
> > > > > > 
> > > > > The fact that kernel does not check them for zero value is what will
> > > > > prevents us from doing so.
> > > > 
> > > > Well we can not change kernel now (it would break userspace)
> > > > but we can start zeroing everything in userspace.
> > > > 
> > > > Also, checkers like coverity might get confused by this.
> > > > 
> > > > Finally, simpler assignment and comparison make it worth it,
> > > > don't they?
> > > > 
> > > I am not at all against the patch! Just pointing out a mistake in the
> > > commit message.
> > 
> > I think we can agree both userspace not initializing them and kernel not
> > checking it are mistakes?
> > 
> Mistake that cannot be fixed at this point.
> 
> > Anyway ... could you commit this tweaking the commit
> > message in a way that you consider appropriate?
> > Or want me to repost?
> > 
> No need to report.

Thanks.

> > 
> > > > > > It's better to just make sure all input is initialized.
> > > > > > 
> > > > > > Once it is, we can also drop complex field by field assignment and just
> > > > > > do the simple *a = *b to update a route entry.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >  kvm-all.c | 19 +++++++------------
> > > > > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > > > > 
> > > > > > diff --git a/kvm-all.c b/kvm-all.c
> > > > > > index 405480e..f119ce1 100644
> > > > > > --- a/kvm-all.c
> > > > > > +++ b/kvm-all.c
> > > > > > @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
> > > > > >      }
> > > > > >      n = s->irq_routes->nr++;
> > > > > >      new = &s->irq_routes->entries[n];
> > > > > > -    memset(new, 0, sizeof(*new));
> > > > > > -    new->gsi = entry->gsi;
> > > > > > -    new->type = entry->type;
> > > > > > -    new->flags = entry->flags;
> > > > > > -    new->u = entry->u;
> > > > > > +
> > > > > > +    *new = *entry;
> > > > > >  
> > > > > >      set_gsi(s, entry->gsi);
> > > > > >  
> > > > > > @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > > > > >              continue;
> > > > > >          }
> > > > > >  
> > > > > > -        entry->type = new_entry->type;
> > > > > > -        entry->flags = new_entry->flags;
> > > > > > -        entry->u = new_entry->u;
> > > > > > +        *entry = *new_entry;
> > > > > >  
> > > > > >          kvm_irqchip_commit_routes(s);
> > > > > >  
> > > > > > @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
> > > > > >  
> > > > > >  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> > > > > >  {
> > > > > > -    struct kvm_irq_routing_entry e;
> > > > > > +    struct kvm_irq_routing_entry e = {};
> > > > > >  
> > > > > >      assert(pin < s->gsi_count);
> > > > > >  
> > > > > > @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > > > > >              return virq;
> > > > > >          }
> > > > > >  
> > > > > > -        route = g_malloc(sizeof(KVMMSIRoute));
> > > > > > +        route = g_malloc0(sizeof(KVMMSIRoute));
> > > > > >          route->kroute.gsi = virq;
> > > > > >          route->kroute.type = KVM_IRQ_ROUTING_MSI;
> > > > > >          route->kroute.flags = 0;
> > > > > > @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> > > > > >  
> > > > > >  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > > > > >  {
> > > > > > -    struct kvm_irq_routing_entry kroute;
> > > > > > +    struct kvm_irq_routing_entry kroute = {};
> > > > > >      int virq;
> > > > > >  
> > > > > >      if (!kvm_gsi_routing_enabled()) {
> > > > > > @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> > > > > >  
> > > > > >  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
> > > > > >  {
> > > > > > -    struct kvm_irq_routing_entry kroute;
> > > > > > +    struct kvm_irq_routing_entry kroute = {};
> > > > > >  
> > > > > >      if (!kvm_irqchip_in_kernel()) {
> > > > > >          return -ENOSYS;
> > > > > > -- 
> > > > > > MST
> > > > > 
> > > > > --
> > > > > 			Gleb.
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Gleb Natapov]

On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> kvm_add_routing_entry makes an attempt to
> zero-initialize any new routing entry.
> However, it fails to initialize padding
> within the u field of the structure
> kvm_irq_routing_entry.
> 
> Other functions like kvm_irqchip_update_msi_route
> also fail to initialize the padding field in
> kvm_irq_routing_entry.
> 
> While mostly harmless, this would prevent us from
> reusing these fields for something useful in
> the future.
> 
> It's better to just make sure all input is initialized.
> 
> Once it is, we can also drop complex field by field assignment and just
> do the simple *a = *b to update a route entry.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied, thanks.

> ---
>  kvm-all.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 405480e..f119ce1 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
>      }
>      n = s->irq_routes->nr++;
>      new = &s->irq_routes->entries[n];
> -    memset(new, 0, sizeof(*new));
> -    new->gsi = entry->gsi;
> -    new->type = entry->type;
> -    new->flags = entry->flags;
> -    new->u = entry->u;
> +
> +    *new = *entry;
>  
>      set_gsi(s, entry->gsi);
>  
> @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
>              continue;
>          }
>  
> -        entry->type = new_entry->type;
> -        entry->flags = new_entry->flags;
> -        entry->u = new_entry->u;
> +        *entry = *new_entry;
>  
>          kvm_irqchip_commit_routes(s);
>  
> @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
>  
>  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
>  {
> -    struct kvm_irq_routing_entry e;
> +    struct kvm_irq_routing_entry e = {};
>  
>      assert(pin < s->gsi_count);
>  
> @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
>              return virq;
>          }
>  
> -        route = g_malloc(sizeof(KVMMSIRoute));
> +        route = g_malloc0(sizeof(KVMMSIRoute));
>          route->kroute.gsi = virq;
>          route->kroute.type = KVM_IRQ_ROUTING_MSI;
>          route->kroute.flags = 0;
> @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
>  
>  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
>  {
> -    struct kvm_irq_routing_entry kroute;
> +    struct kvm_irq_routing_entry kroute = {};
>      int virq;
>  
>      if (!kvm_gsi_routing_enabled()) {
> @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
>  
>  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
>  {
> -    struct kvm_irq_routing_entry kroute;
> +    struct kvm_irq_routing_entry kroute = {};
>  
>      if (!kvm_irqchip_in_kernel()) {
>          return -ENOSYS;
> -- 
> MST

--
			Gleb.