1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/intc: Handle software disabling of APIC correctly

Jay Khandkar posted 1 patch 3 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220712141804.99494-1-jaykhandkar2002@gmail.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
hw/intc/apic.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
[PATCH] hw/intc: Handle software disabling of APIC correctly
Posted by Jay Khandkar 3 years, 9 months ago 
When the local APIC is in a software disabled state, all local interrupt
sources must be masked and all attempts to unmask them should be
ignored. Currently, we don't do either. Fix this by handling it
correctly in apic_mem_write().

Signed-off-by: Jay Khandkar <jaykhandkar2002@gmail.com>
---
 hw/intc/apic.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 3df11c34d6..493c70af62 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -792,9 +792,16 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         s->dest_mode = val >> 28;
         break;
     case 0x0f:
-        s->spurious_vec = val & 0x1ff;
-        apic_update_irq(s);
-        break;
+        {
+            s->spurious_vec = val & 0x1ff;
+            if (!(val & APIC_SPURIO_ENABLED)) {
+                for (int i = 0; i < APIC_LVT_NB; i++) {
+                    s->lvt[i] |= APIC_LVT_MASKED;
+                }
+            }
+            apic_update_irq(s);
+            break;
+        }
     case 0x10 ... 0x17:
     case 0x18 ... 0x1f:
     case 0x20 ... 0x27:
@@ -812,6 +819,9 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
     case 0x32 ... 0x37:
         {
             int n = index - 0x32;
+            if (!(s->spurious_vec & APIC_SPURIO_ENABLED)) {
+                val |= APIC_LVT_MASKED;
+            }
             s->lvt[n] = val;
             if (n == APIC_LVT_TIMER) {
                 apic_timer_update(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-- 
2.37.0
Re: [PATCH] hw/intc: Handle software disabling of APIC correctly
Posted by Peter Maydell 3 years, 9 months ago 
On Tue, 12 Jul 2022 at 19:38, Jay Khandkar <jaykhandkar2002@gmail.com> wrote:
>
> When the local APIC is in a software disabled state, all local interrupt
> sources must be masked and all attempts to unmask them should be
> ignored. Currently, we don't do either. Fix this by handling it
> correctly in apic_mem_write().
>
> Signed-off-by: Jay Khandkar <jaykhandkar2002@gmail.com>
> ---
>  hw/intc/apic.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 3df11c34d6..493c70af62 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -792,9 +792,16 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>          s->dest_mode = val >> 28;
>          break;
>      case 0x0f:
> -        s->spurious_vec = val & 0x1ff;
> -        apic_update_irq(s);
> -        break;
> +        {
> +            s->spurious_vec = val & 0x1ff;
> +            if (!(val & APIC_SPURIO_ENABLED)) {
> +                for (int i = 0; i < APIC_LVT_NB; i++) {
> +                    s->lvt[i] |= APIC_LVT_MASKED;
> +                }
> +            }
> +            apic_update_irq(s);
> +            break;
> +        }

What are the braces for here ? There's no local variable declaration...

thanks
-- PMM
Re: [PATCH] hw/intc: Handle software disabling of APIC correctly
Posted by Jay Khandkar 3 years, 9 months ago 
On Fri, Jul 29, 2022 at 06:09:01PM +0100, Peter Maydell wrote:
> On Tue, 12 Jul 2022 at 19:38, Jay Khandkar <jaykhandkar2002@gmail.com> wrote:
> >
> > When the local APIC is in a software disabled state, all local interrupt
> > sources must be masked and all attempts to unmask them should be
> > ignored. Currently, we don't do either. Fix this by handling it
> > correctly in apic_mem_write().
> >
> > Signed-off-by: Jay Khandkar <jaykhandkar2002@gmail.com>
> > ---
> >  hw/intc/apic.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> > index 3df11c34d6..493c70af62 100644
> > --- a/hw/intc/apic.c
> > +++ b/hw/intc/apic.c
> > @@ -792,9 +792,16 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
> >          s->dest_mode = val >> 28;
> >          break;
> >      case 0x0f:
> > -        s->spurious_vec = val & 0x1ff;
> > -        apic_update_irq(s);
> > -        break;
> > +        {
> > +            s->spurious_vec = val & 0x1ff;
> > +            if (!(val & APIC_SPURIO_ENABLED)) {
> > +                for (int i = 0; i < APIC_LVT_NB; i++) {
> > +                    s->lvt[i] |= APIC_LVT_MASKED;
> > +                }
> > +            }
> > +            apic_update_irq(s);
> > +            break;
> > +        }
> 
> What are the braces for here ? There's no local variable declaration...
> 
> thanks
> -- PMM
You are right, the braces are unnecessary for that part. I just put them in to
create a neat visually separate block. Can get rid of them.

Thanks,
Jay
Re: [PATCH] hw/intc: Handle software disabling of APIC correctly
Posted by Michael S. Tsirkin 3 years, 8 months ago 
On Fri, Jul 29, 2022 at 11:04:47PM +0530, Jay Khandkar wrote:
> On Fri, Jul 29, 2022 at 06:09:01PM +0100, Peter Maydell wrote:
> > On Tue, 12 Jul 2022 at 19:38, Jay Khandkar <jaykhandkar2002@gmail.com> wrote:
> > >
> > > When the local APIC is in a software disabled state, all local interrupt
> > > sources must be masked and all attempts to unmask them should be
> > > ignored. Currently, we don't do either. Fix this by handling it
> > > correctly in apic_mem_write().
> > >
> > > Signed-off-by: Jay Khandkar <jaykhandkar2002@gmail.com>
> > > ---
> > >  hw/intc/apic.c | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> > > index 3df11c34d6..493c70af62 100644
> > > --- a/hw/intc/apic.c
> > > +++ b/hw/intc/apic.c
> > > @@ -792,9 +792,16 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
> > >          s->dest_mode = val >> 28;
> > >          break;
> > >      case 0x0f:
> > > -        s->spurious_vec = val & 0x1ff;
> > > -        apic_update_irq(s);
> > > -        break;
> > > +        {
> > > +            s->spurious_vec = val & 0x1ff;
> > > +            if (!(val & APIC_SPURIO_ENABLED)) {
> > > +                for (int i = 0; i < APIC_LVT_NB; i++) {
> > > +                    s->lvt[i] |= APIC_LVT_MASKED;
> > > +                }
> > > +            }
> > > +            apic_update_irq(s);
> > > +            break;
> > > +        }
> > 
> > What are the braces for here ? There's no local variable declaration...
> > 
> > thanks
> > -- PMM
> You are right, the braces are unnecessary for that part. I just put them in to
> create a neat visually separate block. Can get rid of them.
> 
> Thanks,
> Jay 

Did you intend to send v2 of this?

-- 
MST
Re: [PATCH] hw/intc: Handle software disabling of APIC correctly
Posted by Jay Khandkar 3 years, 8 months ago 
Have sent in v2.

On Wed, Aug 17, 2022 at 4:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Fri, Jul 29, 2022 at 11:04:47PM +0530, Jay Khandkar wrote:
> > On Fri, Jul 29, 2022 at 06:09:01PM +0100, Peter Maydell wrote:
> > > On Tue, 12 Jul 2022 at 19:38, Jay Khandkar <jaykhandkar2002@gmail.com>
> wrote:
> > > >
> > > > When the local APIC is in a software disabled state, all local
> interrupt
> > > > sources must be masked and all attempts to unmask them should be
> > > > ignored. Currently, we don't do either. Fix this by handling it
> > > > correctly in apic_mem_write().
> > > >
> > > > Signed-off-by: Jay Khandkar <jaykhandkar2002@gmail.com>
> > > > ---
> > > >  hw/intc/apic.c | 16 +++++++++++++---
> > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> > > > index 3df11c34d6..493c70af62 100644
> > > > --- a/hw/intc/apic.c
> > > > +++ b/hw/intc/apic.c
> > > > @@ -792,9 +792,16 @@ static void apic_mem_write(void *opaque, hwaddr
> addr, uint64_t val,
> > > >          s->dest_mode = val >> 28;
> > > >          break;
> > > >      case 0x0f:
> > > > -        s->spurious_vec = val & 0x1ff;
> > > > -        apic_update_irq(s);
> > > > -        break;
> > > > +        {
> > > > +            s->spurious_vec = val & 0x1ff;
> > > > +            if (!(val & APIC_SPURIO_ENABLED)) {
> > > > +                for (int i = 0; i < APIC_LVT_NB; i++) {
> > > > +                    s->lvt[i] |= APIC_LVT_MASKED;
> > > > +                }
> > > > +            }
> > > > +            apic_update_irq(s);
> > > > +            break;
> > > > +        }
> > >
> > > What are the braces for here ? There's no local variable declaration...
> > >
> > > thanks
> > > -- PMM
> > You are right, the braces are unnecessary for that part. I just put them
> in to
> > create a neat visually separate block. Can get rid of them.
> >
> > Thanks,
> > Jay
>
> Did you intend to send v2 of this?
>
> --
> MST
>
>
Re: [PATCH] hw/intc: Handle software disabling of APIC correctly
Posted by Jay Khandkar 3 years, 9 months ago 
Ping?

On Tue, 12 Jul 2022, 19:49 Jay Khandkar, <jaykhandkar2002@gmail.com> wrote:

> When the local APIC is in a software disabled state, all local interrupt
> sources must be masked and all attempts to unmask them should be
> ignored. Currently, we don't do either. Fix this by handling it
> correctly in apic_mem_write().
>
> Signed-off-by: Jay Khandkar <jaykhandkar2002@gmail.com>
> ---
>  hw/intc/apic.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 3df11c34d6..493c70af62 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -792,9 +792,16 @@ static void apic_mem_write(void *opaque, hwaddr addr,
> uint64_t val,
>          s->dest_mode = val >> 28;
>          break;
>      case 0x0f:
> -        s->spurious_vec = val & 0x1ff;
> -        apic_update_irq(s);
> -        break;
> +        {
> +            s->spurious_vec = val & 0x1ff;
> +            if (!(val & APIC_SPURIO_ENABLED)) {
> +                for (int i = 0; i < APIC_LVT_NB; i++) {
> +                    s->lvt[i] |= APIC_LVT_MASKED;
> +                }
> +            }
> +            apic_update_irq(s);
> +            break;
> +        }
>      case 0x10 ... 0x17:
>      case 0x18 ... 0x1f:
>      case 0x20 ... 0x27:
> @@ -812,6 +819,9 @@ static void apic_mem_write(void *opaque, hwaddr addr,
> uint64_t val,
>      case 0x32 ... 0x37:
>          {
>              int n = index - 0x32;
> +            if (!(s->spurious_vec & APIC_SPURIO_ENABLED)) {
> +                val |= APIC_LVT_MASKED;
> +            }
>              s->lvt[n] = val;
>              if (n == APIC_LVT_TIMER) {
>                  apic_timer_update(s,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> --
> 2.37.0
>
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.