[Qemu-devel] [PATCH] spapr/xive: H_INT_ESB is used for LSIs only

[Qemu-devel] [PATCH] spapr/xive: H_INT_ESB is used for LSIs only

[Greg Kurz]

As indicated in the function header, this "hcall is only supported for
LISNs that have the ESB hcall flag set to 1 when returned from hcall()
H_INT_GET_SOURCE_INFO". We only set that flag for LSIs actually.

Check that in h_int_esb().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 58c2e5d890bd..01dd47ad5b02 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1408,6 +1408,12 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
         return H_P2;
     }
 
+    if (!xive_source_irq_is_lsi(xsrc, lisn)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: LISN " TARGET_FMT_lx "isn't LSI\n",
+                      lisn);
+        return H_P2;
+    }
+
     if (offset > (1ull << xsrc->esb_shift)) {
         return H_P3;
     }

Re: [Qemu-devel] [PATCH] spapr/xive: H_INT_ESB is used for LSIs only

[Cédric Le Goater]

On 21/06/2019 16:52, Greg Kurz wrote:
> As indicated in the function header, this "hcall is only supported for
> LISNs that have the ESB hcall flag set to 1 when returned from hcall()
> H_INT_GET_SOURCE_INFO". We only set that flag for LSIs actually.
> 
> Check that in h_int_esb().

Indeed. H_INT_ESB should work on any IRQ, but I think it's better 
to check that the HCALL is only used with the IRQ requiring it.   

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/spapr_xive.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 58c2e5d890bd..01dd47ad5b02 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1408,6 +1408,12 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
>          return H_P2;
>      }
>  
> +    if (!xive_source_irq_is_lsi(xsrc, lisn)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: LISN " TARGET_FMT_lx "isn't LSI\n",
> +                      lisn);
> +        return H_P2;
> +    }
> +
>      if (offset > (1ull << xsrc->esb_shift)) {
>          return H_P3;
>      }
> 

Re: [Qemu-devel] [PATCH] spapr/xive: H_INT_ESB is used for LSIs only

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

On Fri, Jun 21, 2019 at 05:05:45PM +0200, Cédric Le Goater wrote:
> On 21/06/2019 16:52, Greg Kurz wrote:
> > As indicated in the function header, this "hcall is only supported for
> > LISNs that have the ESB hcall flag set to 1 when returned from hcall()
> > H_INT_GET_SOURCE_INFO". We only set that flag for LSIs actually.
> > 
> > Check that in h_int_esb().
> 
> Indeed. H_INT_ESB should work on any IRQ, but I think it's better 
> to check that the HCALL is only used with the IRQ requiring it.

I'm not so convinced.  It seems to me that specifically limiting this
to certain things limits our options if we ever need future
workarounds for problems with ESB mapping.

In addition using H_INT_ESB for non-LSIs could be useful for minimal
guests (e.g. kvm-unit-tests) where mapping memory might be awkward.

So, I'm not really seeing a compelling reason to apply this.

> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> > ---
> >  hw/intc/spapr_xive.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 58c2e5d890bd..01dd47ad5b02 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -1408,6 +1408,12 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
> >          return H_P2;
> >      }
> >  
> > +    if (!xive_source_irq_is_lsi(xsrc, lisn)) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: LISN " TARGET_FMT_lx "isn't LSI\n",
> > +                      lisn);
> > +        return H_P2;
> > +    }
> > +
> >      if (offset > (1ull << xsrc->esb_shift)) {
> >          return H_P3;
> >      }
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] spapr/xive: H_INT_ESB is used for LSIs only

[Cédric Le Goater]

On 01/07/2019 07:07, David Gibson wrote:
> On Fri, Jun 21, 2019 at 05:05:45PM +0200, Cédric Le Goater wrote:
>> On 21/06/2019 16:52, Greg Kurz wrote:
>>> As indicated in the function header, this "hcall is only supported for
>>> LISNs that have the ESB hcall flag set to 1 when returned from hcall()
>>> H_INT_GET_SOURCE_INFO". We only set that flag for LSIs actually.
>>>
>>> Check that in h_int_esb().
>>
>> Indeed. H_INT_ESB should work on any IRQ, but I think it's better 
>> to check that the HCALL is only used with the IRQ requiring it.
> 
> I'm not so convinced.  It seems to me that specifically limiting this
> to certain things limits our options if we ever need future
> workarounds for problems with ESB mapping.
> 
> In addition using H_INT_ESB for non-LSIs could be useful for minimal
> guests (e.g. kvm-unit-tests) where mapping memory might be awkward.

Ah yes. This is true. There is no real reason for enforcing this
restriction in QEMU as H_INT_ESB should work on any irq. We can
keep it that way I guess. It would be a small deviation from PAPR.

Thanks,

C. 

> So, I'm not really seeing a compelling reason to apply this.
> 
>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Thanks,
>>
>> C.
>>
>>> ---
>>>  hw/intc/spapr_xive.c |    6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>>> index 58c2e5d890bd..01dd47ad5b02 100644
>>> --- a/hw/intc/spapr_xive.c
>>> +++ b/hw/intc/spapr_xive.c
>>> @@ -1408,6 +1408,12 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
>>>          return H_P2;
>>>      }
>>>  
>>> +    if (!xive_source_irq_is_lsi(xsrc, lisn)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: LISN " TARGET_FMT_lx "isn't LSI\n",
>>> +                      lisn);
>>> +        return H_P2;
>>> +    }
>>> +
>>>      if (offset > (1ull << xsrc->esb_shift)) {
>>>          return H_P3;
>>>      }
>>>
>>
> 

Re: [Qemu-devel] [PATCH] spapr/xive: H_INT_ESB is used for LSIs only

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On Mon, Jul 01, 2019 at 07:55:03AM +0200, Cédric Le Goater wrote:
> On 01/07/2019 07:07, David Gibson wrote:
> > On Fri, Jun 21, 2019 at 05:05:45PM +0200, Cédric Le Goater wrote:
> >> On 21/06/2019 16:52, Greg Kurz wrote:
> >>> As indicated in the function header, this "hcall is only supported for
> >>> LISNs that have the ESB hcall flag set to 1 when returned from hcall()
> >>> H_INT_GET_SOURCE_INFO". We only set that flag for LSIs actually.
> >>>
> >>> Check that in h_int_esb().
> >>
> >> Indeed. H_INT_ESB should work on any IRQ, but I think it's better 
> >> to check that the HCALL is only used with the IRQ requiring it.
> > 
> > I'm not so convinced.  It seems to me that specifically limiting this
> > to certain things limits our options if we ever need future
> > workarounds for problems with ESB mapping.
> > 
> > In addition using H_INT_ESB for non-LSIs could be useful for minimal
> > guests (e.g. kvm-unit-tests) where mapping memory might be awkward.
> 
> Ah yes. This is true. There is no real reason for enforcing this
> restriction in QEMU as H_INT_ESB should work on any irq. We can
> keep it that way I guess. It would be a small deviation from PAPR.

True, but "unexpectedly working" is a form of variation exceedingly
unlikely to break anything.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]