| Message ID | 156112873945.115975.15513090884722011930.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr/xive: H_INT_ESB is used for LSIs only | expand |
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; > } >
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; > > } > > >
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; >>> } >>> >> >
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.
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; }
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(+)