| Message ID | 20171211134740.8235-7-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: flic rework, tcg flic support and tcg | expand |
On Mon, 11 Dec 2017 14:47:31 +0100 David Hildenbrand <david@redhat.com> wrote: > Let the flic device handle it internally. This will allow us to later > on store floating interrupts in the flic for the TCG case. > > This now also simplifies kvm.c. All that's left is the fallback > interface for floating interrupts, which is no triggered directly via s/no/now/ > the flic in case anything goes wrong. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/intc/s390_flic.c | 31 ++++++++++++++++++++ > hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- > include/hw/s390x/s390_flic.h | 5 ++++ > target/s390x/cpu.h | 7 ++++- > target/s390x/interrupt.c | 42 +++++++++++---------------- > target/s390x/kvm-stub.c | 13 --------- > target/s390x/kvm.c | 68 ++++---------------------------------------- > target/s390x/kvm_s390x.h | 10 +------ > 8 files changed, 123 insertions(+), 116 deletions(-) > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > index a78bdf1d90..8d521c415a 100644 > --- a/hw/intc/s390_flic.c > +++ b/hw/intc/s390_flic.c > @@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type, > return 0; > } > > +static void qemu_s390_inject_service(S390FLICState *fs, uint32_t parm) > +{ > + > + S390CPU *dummy_cpu = s390_cpu_addr2state(0); > + > + /* FIXME: don't inject into dummy CPU */ > + cpu_inject_service(dummy_cpu, parm); > +} > + > +static void qemu_s390_inject_io(S390FLICState *fs, uint16_t subchannel_id, > + uint16_t subchannel_nr, uint32_t io_int_parm, > + uint32_t io_int_word) > +{ > + S390CPU *dummy_cpu = s390_cpu_addr2state(0); > + > + /* FIXME: don't inject into dummy CPU */ > + cpu_inject_io(dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, > + io_int_word); > +} > + > +static void qemu_s390_inject_crw_mchk(S390FLICState *fs) > +{ > + S390CPU *dummy_cpu = s390_cpu_addr2state(0); > + > + /* FIXME: don't inject into dummy CPU */ > + cpu_inject_crw_mchk(dummy_cpu); > +} > + > static void qemu_s390_flic_reset(DeviceState *dev) > { > QEMUS390FLICState *flic = QEMU_S390_FLIC(dev); > @@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) > fsc->clear_io_irq = qemu_s390_clear_io_flic; > fsc->modify_ais_mode = qemu_s390_modify_ais_mode; > fsc->inject_airq = qemu_s390_inject_airq; > + fsc->inject_service = qemu_s390_inject_service; > + fsc->inject_io = qemu_s390_inject_io; > + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; > } As you now have a callback for ->inject_io(), make qemu_s390_inject_airq() invoke it directly instead of going the detour through s390_io_interrupt()? > > static Property s390_flic_common_properties[] = { (...) Generally looks sane. One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
On 12/12/2017 02:49 PM, Cornelia Huck wrote: > On Mon, 11 Dec 2017 14:47:31 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> Let the flic device handle it internally. This will allow us to later >> on store floating interrupts in the flic for the TCG case. >> >> This now also simplifies kvm.c. All that's left is the fallback >> interface for floating interrupts, which is no triggered directly via > > s/no/now/ > >> the flic in case anything goes wrong. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/intc/s390_flic.c | 31 ++++++++++++++++++++ >> hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- >> include/hw/s390x/s390_flic.h | 5 ++++ >> target/s390x/cpu.h | 7 ++++- >> target/s390x/interrupt.c | 42 +++++++++++---------------- >> target/s390x/kvm-stub.c | 13 --------- >> target/s390x/kvm.c | 68 ++++---------------------------------------- >> target/s390x/kvm_s390x.h | 10 +------ >> 8 files changed, 123 insertions(+), 116 deletions(-) >> >> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c >> index a78bdf1d90..8d521c415a 100644 >> --- a/hw/intc/s390_flic.c >> +++ b/hw/intc/s390_flic.c >> @@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type, >> return 0; >> } >> >> +static void qemu_s390_inject_service(S390FLICState *fs, uint32_t parm) >> +{ >> + >> + S390CPU *dummy_cpu = s390_cpu_addr2state(0); >> + >> + /* FIXME: don't inject into dummy CPU */ >> + cpu_inject_service(dummy_cpu, parm); >> +} >> + >> +static void qemu_s390_inject_io(S390FLICState *fs, uint16_t subchannel_id, >> + uint16_t subchannel_nr, uint32_t io_int_parm, >> + uint32_t io_int_word) >> +{ >> + S390CPU *dummy_cpu = s390_cpu_addr2state(0); >> + >> + /* FIXME: don't inject into dummy CPU */ >> + cpu_inject_io(dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, >> + io_int_word); >> +} >> + >> +static void qemu_s390_inject_crw_mchk(S390FLICState *fs) >> +{ >> + S390CPU *dummy_cpu = s390_cpu_addr2state(0); >> + >> + /* FIXME: don't inject into dummy CPU */ >> + cpu_inject_crw_mchk(dummy_cpu); >> +} >> + >> static void qemu_s390_flic_reset(DeviceState *dev) >> { >> QEMUS390FLICState *flic = QEMU_S390_FLIC(dev); >> @@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) >> fsc->clear_io_irq = qemu_s390_clear_io_flic; >> fsc->modify_ais_mode = qemu_s390_modify_ais_mode; >> fsc->inject_airq = qemu_s390_inject_airq; >> + fsc->inject_service = qemu_s390_inject_service; >> + fsc->inject_io = qemu_s390_inject_io; >> + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; >> } > > As you now have a callback for ->inject_io(), make > qemu_s390_inject_airq() invoke it directly instead of going the detour > through s390_io_interrupt()? > >> >> static Property s390_flic_common_properties[] = { > > (...) > > Generally looks sane. > > One thing I noticed: You removed the caching of the flic (in the old > kvm inject routine), and you generally do more qom invocations (first, > to find the common flic; then, to translate to the qemu or kvm flic). > Not sure if this might be a problem (probably not). Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 12/12/2017 02:49 PM, Cornelia Huck wrote: > > One thing I noticed: You removed the caching of the flic (in the old > > kvm inject routine), and you generally do more qom invocations (first, > > to find the common flic; then, to translate to the qemu or kvm flic). > > Not sure if this might be a problem (probably not). > > Is any of these calls on a potential fast path (e.g. guest without adapter > interrupts)? If yes, then QOM is a no-go since it is really slow. At least the new airq interface was using QOM without caching before. It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
On 12.12.2017 15:29, Cornelia Huck wrote: > On Tue, 12 Dec 2017 15:13:46 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 12/12/2017 02:49 PM, Cornelia Huck wrote: > >>> One thing I noticed: You removed the caching of the flic (in the old >>> kvm inject routine), and you generally do more qom invocations (first, >>> to find the common flic; then, to translate to the qemu or kvm flic). >>> Not sure if this might be a problem (probably not). >> >> Is any of these calls on a potential fast path (e.g. guest without adapter >> interrupts)? If yes, then QOM is a no-go since it is really slow. > > At least the new airq interface was using QOM without caching before. > > It's basically about any interrupt; but otoh we are (for kvm) in > userspace already. Caching the flic and just keeping the casting to the > specialized flic might be ok (I'd guess that the lookup is the slowest > path.) > Please note that the lookup is already cached in s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no? The other conversions should be cheap (and already were in place in a couple of places before). Thanks!
On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <david@redhat.com> wrote: > On 12.12.2017 15:29, Cornelia Huck wrote: > > On Tue, 12 Dec 2017 15:13:46 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> On 12/12/2017 02:49 PM, Cornelia Huck wrote: > > > >>> One thing I noticed: You removed the caching of the flic (in the old > >>> kvm inject routine), and you generally do more qom invocations (first, > >>> to find the common flic; then, to translate to the qemu or kvm flic). > >>> Not sure if this might be a problem (probably not). > >> > >> Is any of these calls on a potential fast path (e.g. guest without adapter > >> interrupts)? If yes, then QOM is a no-go since it is really slow. > > > > At least the new airq interface was using QOM without caching before. > > > > It's basically about any interrupt; but otoh we are (for kvm) in > > userspace already. Caching the flic and just keeping the casting to the > > specialized flic might be ok (I'd guess that the lookup is the slowest > > path.) > > > > Please note that the lookup is already cached in s390_get_flic(); That > should be sufficient, as it does the expensive lookup. One cache should > be enough, no? Ah, missed that. So the old code actually did double caching... > > The other conversions should be cheap (and already were in place in a > couple of places before). Yes, object_resolve_path() is probably the most expensive one. Did anyone ever check if the (existing) conversions are actually measurable?
On 12.12.2017 14:49, Cornelia Huck wrote: > On Mon, 11 Dec 2017 14:47:31 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> Let the flic device handle it internally. This will allow us to later >> on store floating interrupts in the flic for the TCG case. >> >> This now also simplifies kvm.c. All that's left is the fallback >> interface for floating interrupts, which is no triggered directly via > > s/no/now/ > >> the flic in case anything goes wrong. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/intc/s390_flic.c | 31 ++++++++++++++++++++ >> hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- >> include/hw/s390x/s390_flic.h | 5 ++++ >> target/s390x/cpu.h | 7 ++++- >> target/s390x/interrupt.c | 42 +++++++++++---------------- >> target/s390x/kvm-stub.c | 13 --------- >> target/s390x/kvm.c | 68 ++++---------------------------------------- >> target/s390x/kvm_s390x.h | 10 +------ >> 8 files changed, 123 insertions(+), 116 deletions(-) >> >> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c >> index a78bdf1d90..8d521c415a 100644 >> --- a/hw/intc/s390_flic.c >> +++ b/hw/intc/s390_flic.c >> @@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type, >> return 0; >> } >> >> +static void qemu_s390_inject_service(S390FLICState *fs, uint32_t parm) >> +{ >> + >> + S390CPU *dummy_cpu = s390_cpu_addr2state(0); >> + >> + /* FIXME: don't inject into dummy CPU */ >> + cpu_inject_service(dummy_cpu, parm); >> +} >> + >> +static void qemu_s390_inject_io(S390FLICState *fs, uint16_t subchannel_id, >> + uint16_t subchannel_nr, uint32_t io_int_parm, >> + uint32_t io_int_word) >> +{ >> + S390CPU *dummy_cpu = s390_cpu_addr2state(0); >> + >> + /* FIXME: don't inject into dummy CPU */ >> + cpu_inject_io(dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, >> + io_int_word); >> +} >> + >> +static void qemu_s390_inject_crw_mchk(S390FLICState *fs) >> +{ >> + S390CPU *dummy_cpu = s390_cpu_addr2state(0); >> + >> + /* FIXME: don't inject into dummy CPU */ >> + cpu_inject_crw_mchk(dummy_cpu); >> +} >> + >> static void qemu_s390_flic_reset(DeviceState *dev) >> { >> QEMUS390FLICState *flic = QEMU_S390_FLIC(dev); >> @@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) >> fsc->clear_io_irq = qemu_s390_clear_io_flic; >> fsc->modify_ais_mode = qemu_s390_modify_ais_mode; >> fsc->inject_airq = qemu_s390_inject_airq; >> + fsc->inject_service = qemu_s390_inject_service; >> + fsc->inject_io = qemu_s390_inject_io; >> + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; >> } > > As you now have a callback for ->inject_io(), make > qemu_s390_inject_airq() invoke it directly instead of going the detour > through s390_io_interrupt()? Indeed, that makes sense.
On 12/12/2017 04:28 PM, Cornelia Huck wrote: > On Tue, 12 Dec 2017 16:17:17 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 12.12.2017 15:29, Cornelia Huck wrote: >>> On Tue, 12 Dec 2017 15:13:46 +0100 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>> >>>> On 12/12/2017 02:49 PM, Cornelia Huck wrote: >>> >>>>> One thing I noticed: You removed the caching of the flic (in the old >>>>> kvm inject routine), and you generally do more qom invocations (first, >>>>> to find the common flic; then, to translate to the qemu or kvm flic). >>>>> Not sure if this might be a problem (probably not). >>>> >>>> Is any of these calls on a potential fast path (e.g. guest without adapter >>>> interrupts)? If yes, then QOM is a no-go since it is really slow. >>> >>> At least the new airq interface was using QOM without caching before. >>> >>> It's basically about any interrupt; but otoh we are (for kvm) in >>> userspace already. Caching the flic and just keeping the casting to the >>> specialized flic might be ok (I'd guess that the lookup is the slowest >>> path.) >>> >> >> Please note that the lookup is already cached in s390_get_flic(); That >> should be sufficient, as it does the expensive lookup. One cache should >> be enough, no? > > Ah, missed that. So the old code actually did double caching... > >> >> The other conversions should be cheap (and already were in place in a >> couple of places before). > > Yes, object_resolve_path() is probably the most expensive one. > > Did anyone ever check if the (existing) conversions are actually > measurable? Did some quick measurement. the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. (measuring s390_get_flic function). The other conversions (S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);) takes about 15-25ns (up to 100 cycles). For irqfd users this does not matter, but things like plan9fs might benefit if we speedup this lookup as well? PS: We can improve the initial object_resolve_path by doing the resolve not for TYPE_KVM_S390_FLIC but "/machine/" TYPE_KVM_S390_FLIC (2500ns instead of 20000) but its still way too slow.
On 13.12.2017 10:16, Christian Borntraeger wrote: > > > On 12/12/2017 04:28 PM, Cornelia Huck wrote: >> On Tue, 12 Dec 2017 16:17:17 +0100 >> David Hildenbrand <david@redhat.com> wrote: >> >>> On 12.12.2017 15:29, Cornelia Huck wrote: >>>> On Tue, 12 Dec 2017 15:13:46 +0100 >>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>> >>>>> On 12/12/2017 02:49 PM, Cornelia Huck wrote: >>>> >>>>>> One thing I noticed: You removed the caching of the flic (in the old >>>>>> kvm inject routine), and you generally do more qom invocations (first, >>>>>> to find the common flic; then, to translate to the qemu or kvm flic). >>>>>> Not sure if this might be a problem (probably not). >>>>> >>>>> Is any of these calls on a potential fast path (e.g. guest without adapter >>>>> interrupts)? If yes, then QOM is a no-go since it is really slow. >>>> >>>> At least the new airq interface was using QOM without caching before. >>>> >>>> It's basically about any interrupt; but otoh we are (for kvm) in >>>> userspace already. Caching the flic and just keeping the casting to the >>>> specialized flic might be ok (I'd guess that the lookup is the slowest >>>> path.) >>>> >>> >>> Please note that the lookup is already cached in s390_get_flic(); That >>> should be sufficient, as it does the expensive lookup. One cache should >>> be enough, no? >> >> Ah, missed that. So the old code actually did double caching... >> >>> >>> The other conversions should be cheap (and already were in place in a >>> couple of places before). >> >> Yes, object_resolve_path() is probably the most expensive one. >> >> Did anyone ever check if the (existing) conversions are actually >> measurable? > > Did some quick measurement. > the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. > (measuring s390_get_flic function). > > > The other conversions (S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);) > takes about 15-25ns (up to 100 cycles). > For irqfd users this does not matter, but things like plan9fs might benefit > if we speedup this lookup as well? Did you also measure the state conversion like QEMU_S390_FLIC() or KVM_S390_FLIC() ? As we call e.g. QEMU_S390_FLIC() on a regular basis in TCG, it might then also make sense to speed that up. a) cache the (converted) state and class in every function. Somewhat uglifies the code. b) introduce new functions that return the cached value Not sure what's better. I prefer to do this as a separate addon patch and can prepare something. > > > PS: We can improve the initial object_resolve_path by doing the resolve not for > TYPE_KVM_S390_FLIC > but > "/machine/" TYPE_KVM_S390_FLIC > (2500ns instead of 20000) > but its still way too slow. > Specifying the absolute path would be even faster I guess. /machine/s390-flic-qemu/ ...
On Wed, 13 Dec 2017 10:16:34 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 12/12/2017 04:28 PM, Cornelia Huck wrote: > > On Tue, 12 Dec 2017 16:17:17 +0100 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 12.12.2017 15:29, Cornelia Huck wrote: > >>> On Tue, 12 Dec 2017 15:13:46 +0100 > >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >>> > >>>> On 12/12/2017 02:49 PM, Cornelia Huck wrote: > >>> > >>>>> One thing I noticed: You removed the caching of the flic (in the old > >>>>> kvm inject routine), and you generally do more qom invocations (first, > >>>>> to find the common flic; then, to translate to the qemu or kvm flic). > >>>>> Not sure if this might be a problem (probably not). > >>>> > >>>> Is any of these calls on a potential fast path (e.g. guest without adapter > >>>> interrupts)? If yes, then QOM is a no-go since it is really slow. > >>> > >>> At least the new airq interface was using QOM without caching before. > >>> > >>> It's basically about any interrupt; but otoh we are (for kvm) in > >>> userspace already. Caching the flic and just keeping the casting to the > >>> specialized flic might be ok (I'd guess that the lookup is the slowest > >>> path.) > >>> > >> > >> Please note that the lookup is already cached in s390_get_flic(); That > >> should be sufficient, as it does the expensive lookup. One cache should > >> be enough, no? > > > > Ah, missed that. So the old code actually did double caching... > > > >> > >> The other conversions should be cheap (and already were in place in a > >> couple of places before). > > > > Yes, object_resolve_path() is probably the most expensive one. > > > > Did anyone ever check if the (existing) conversions are actually > > measurable? > > Did some quick measurement. > the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. > (measuring s390_get_flic function). > > > The other conversions (S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);) > takes about 15-25ns (up to 100 cycles). That's probably the debug checks, the rest should not take many cycles. > For irqfd users this does not matter, but things like plan9fs might benefit > if we speedup this lookup as well? We can probably cache the fsc as well. There are still the casts from the common flic to the qemu/kvm flics; we can cache them in the individual callbacks. Looks a bit odd in the code though, I guess. An alternative would be using "fast" casts that bypass QOM. > > > PS: We can improve the initial object_resolve_path by doing the resolve not for > TYPE_KVM_S390_FLIC > but > "/machine/" TYPE_KVM_S390_FLIC > (2500ns instead of 20000) > but its still way too slow. > Yeah, caching looks like the saner approach in that case.
On 12/13/2017 10:31 AM, David Hildenbrand wrote: > On 13.12.2017 10:16, Christian Borntraeger wrote: >> >> >> On 12/12/2017 04:28 PM, Cornelia Huck wrote: >>> On Tue, 12 Dec 2017 16:17:17 +0100 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 12.12.2017 15:29, Cornelia Huck wrote: >>>>> On Tue, 12 Dec 2017 15:13:46 +0100 >>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>>> >>>>>> On 12/12/2017 02:49 PM, Cornelia Huck wrote: >>>>> >>>>>>> One thing I noticed: You removed the caching of the flic (in the old >>>>>>> kvm inject routine), and you generally do more qom invocations (first, >>>>>>> to find the common flic; then, to translate to the qemu or kvm flic). >>>>>>> Not sure if this might be a problem (probably not). >>>>>> >>>>>> Is any of these calls on a potential fast path (e.g. guest without adapter >>>>>> interrupts)? If yes, then QOM is a no-go since it is really slow. >>>>> >>>>> At least the new airq interface was using QOM without caching before. >>>>> >>>>> It's basically about any interrupt; but otoh we are (for kvm) in >>>>> userspace already. Caching the flic and just keeping the casting to the >>>>> specialized flic might be ok (I'd guess that the lookup is the slowest >>>>> path.) >>>>> >>>> >>>> Please note that the lookup is already cached in s390_get_flic(); That >>>> should be sufficient, as it does the expensive lookup. One cache should >>>> be enough, no? >>> >>> Ah, missed that. So the old code actually did double caching... >>> >>>> >>>> The other conversions should be cheap (and already were in place in a >>>> couple of places before). >>> >>> Yes, object_resolve_path() is probably the most expensive one. >>> >>> Did anyone ever check if the (existing) conversions are actually >>> measurable? >> >> Did some quick measurement. >> the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. >> (measuring s390_get_flic function). >> >> >> The other conversions (S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);) >> takes about 15-25ns (up to 100 cycles). >> For irqfd users this does not matter, but things like plan9fs might benefit >> if we speedup this lookup as well? > > Did you also measure the state conversion like QEMU_S390_FLIC() or > KVM_S390_FLIC() ? somewhat between 4 and 39 ns.
On Wed, 13 Dec 2017 10:31:58 +0100 David Hildenbrand <david@redhat.com> wrote: > On 13.12.2017 10:16, Christian Borntraeger wrote: > > > > > > On 12/12/2017 04:28 PM, Cornelia Huck wrote: > >> On Tue, 12 Dec 2017 16:17:17 +0100 > >> David Hildenbrand <david@redhat.com> wrote: > >> > >>> On 12.12.2017 15:29, Cornelia Huck wrote: > >>>> On Tue, 12 Dec 2017 15:13:46 +0100 > >>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >>>> > >>>>> On 12/12/2017 02:49 PM, Cornelia Huck wrote: > >>>> > >>>>>> One thing I noticed: You removed the caching of the flic (in the old > >>>>>> kvm inject routine), and you generally do more qom invocations (first, > >>>>>> to find the common flic; then, to translate to the qemu or kvm flic). > >>>>>> Not sure if this might be a problem (probably not). > >>>>> > >>>>> Is any of these calls on a potential fast path (e.g. guest without adapter > >>>>> interrupts)? If yes, then QOM is a no-go since it is really slow. > >>>> > >>>> At least the new airq interface was using QOM without caching before. > >>>> > >>>> It's basically about any interrupt; but otoh we are (for kvm) in > >>>> userspace already. Caching the flic and just keeping the casting to the > >>>> specialized flic might be ok (I'd guess that the lookup is the slowest > >>>> path.) > >>>> > >>> > >>> Please note that the lookup is already cached in s390_get_flic(); That > >>> should be sufficient, as it does the expensive lookup. One cache should > >>> be enough, no? > >> > >> Ah, missed that. So the old code actually did double caching... > >> > >>> > >>> The other conversions should be cheap (and already were in place in a > >>> couple of places before). > >> > >> Yes, object_resolve_path() is probably the most expensive one. > >> > >> Did anyone ever check if the (existing) conversions are actually > >> measurable? > > > > Did some quick measurement. > > the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. > > (measuring s390_get_flic function). > > > > > > The other conversions (S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);) > > takes about 15-25ns (up to 100 cycles). > > For irqfd users this does not matter, but things like plan9fs might benefit > > if we speedup this lookup as well? > > Did you also measure the state conversion like QEMU_S390_FLIC() or > KVM_S390_FLIC() ? > > As we call e.g. QEMU_S390_FLIC() on a regular basis in TCG, it might > then also make sense to speed that up. > > a) cache the (converted) state and class in every function. Somewhat > uglifies the code. > > b) introduce new functions that return the cached value > > Not sure what's better. I prefer to do this as a separate addon patch > and can prepare something. If you want to do it as addon, I vote for option b). > > > > > > > PS: We can improve the initial object_resolve_path by doing the resolve not for > > TYPE_KVM_S390_FLIC > > but > > "/machine/" TYPE_KVM_S390_FLIC > > (2500ns instead of 20000) > > but its still way too slow. > > > > Specifying the absolute path would be even faster I guess. > > /machine/s390-flic-qemu/ ... I don't think we really need to speed up the initial lookup.
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c index a78bdf1d90..8d521c415a 100644 --- a/hw/intc/s390_flic.c +++ b/hw/intc/s390_flic.c @@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type, return 0; } +static void qemu_s390_inject_service(S390FLICState *fs, uint32_t parm) +{ + + S390CPU *dummy_cpu = s390_cpu_addr2state(0); + + /* FIXME: don't inject into dummy CPU */ + cpu_inject_service(dummy_cpu, parm); +} + +static void qemu_s390_inject_io(S390FLICState *fs, uint16_t subchannel_id, + uint16_t subchannel_nr, uint32_t io_int_parm, + uint32_t io_int_word) +{ + S390CPU *dummy_cpu = s390_cpu_addr2state(0); + + /* FIXME: don't inject into dummy CPU */ + cpu_inject_io(dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, + io_int_word); +} + +static void qemu_s390_inject_crw_mchk(S390FLICState *fs) +{ + S390CPU *dummy_cpu = s390_cpu_addr2state(0); + + /* FIXME: don't inject into dummy CPU */ + cpu_inject_crw_mchk(dummy_cpu); +} + static void qemu_s390_flic_reset(DeviceState *dev) { QEMUS390FLICState *flic = QEMU_S390_FLIC(dev); @@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) fsc->clear_io_irq = qemu_s390_clear_io_flic; fsc->modify_ais_mode = qemu_s390_modify_ais_mode; fsc->inject_airq = qemu_s390_inject_airq; + fsc->inject_service = qemu_s390_inject_service; + fsc->inject_io = qemu_s390_inject_io; + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; } static Property s390_flic_common_properties[] = { diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c index 0cb5feab0c..d277ffdd2e 100644 --- a/hw/intc/s390_flic_kvm.c +++ b/hw/intc/s390_flic_kvm.c @@ -111,14 +111,64 @@ static int flic_enqueue_irqs(void *buf, uint64_t len, return rc ? -errno : 0; } -int kvm_s390_inject_flic(struct kvm_s390_irq *irq) +static void kvm_s390_inject_flic(S390FLICState *fs, struct kvm_s390_irq *irq) { - static KVMS390FLICState *flic; + static bool use_flic = true; + int r; + + if (use_flic) { + r = flic_enqueue_irqs(irq, sizeof(*irq), KVM_S390_FLIC(fs)); + if (r == -ENOSYS) { + use_flic = false; + } + if (!r) { + return; + } + } + /* fallback to legacy KVM IOCTL in case FLIC fails */ + kvm_s390_floating_interrupt_legacy(irq); +} + +static void kvm_s390_inject_service(S390FLICState *fs, uint32_t parm) +{ + struct kvm_s390_irq irq = { + .type = KVM_S390_INT_SERVICE, + .u.ext.ext_params = parm, + }; + + kvm_s390_inject_flic(fs, &irq); +} - if (unlikely(!flic)) { - flic = KVM_S390_FLIC(s390_get_flic()); +static void kvm_s390_inject_io(S390FLICState *fs, uint16_t subchannel_id, + uint16_t subchannel_nr, uint32_t io_int_parm, + uint32_t io_int_word) +{ + struct kvm_s390_irq irq = { + .u.io.subchannel_id = subchannel_id, + .u.io.subchannel_nr = subchannel_nr, + .u.io.io_int_parm = io_int_parm, + .u.io.io_int_word = io_int_word, + }; + + if (io_int_word & IO_INT_WORD_AI) { + irq.type = KVM_S390_INT_IO(1, 0, 0, 0); + } else { + irq.type = KVM_S390_INT_IO(0, (subchannel_id & 0xff00) >> 8, + (subchannel_id & 0x0006), + subchannel_nr); } - return flic_enqueue_irqs(irq, sizeof(*irq), flic); + kvm_s390_inject_flic(fs, &irq); +} + +static void kvm_s390_inject_crw_mchk(S390FLICState *fs) +{ + struct kvm_s390_irq irq = { + .type = KVM_S390_MCHK, + .u.mchk.cr14 = CR14_CHANNEL_REPORT_SC, + .u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP, + }; + + kvm_s390_inject_flic(fs, &irq); } static int kvm_s390_clear_io_flic(S390FLICState *fs, uint16_t subchannel_id, @@ -602,6 +652,9 @@ static void kvm_s390_flic_class_init(ObjectClass *oc, void *data) fsc->clear_io_irq = kvm_s390_clear_io_flic; fsc->modify_ais_mode = kvm_s390_modify_ais_mode; fsc->inject_airq = kvm_s390_inject_airq; + fsc->inject_service = kvm_s390_inject_service; + fsc->inject_io = kvm_s390_inject_io; + fsc->inject_crw_mchk = kvm_s390_inject_crw_mchk; } static const TypeInfo kvm_s390_flic_info = { diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h index 5b00e936fa..d0538134b7 100644 --- a/include/hw/s390x/s390_flic.h +++ b/include/hw/s390x/s390_flic.h @@ -66,6 +66,11 @@ typedef struct S390FLICStateClass { int (*modify_ais_mode)(S390FLICState *fs, uint8_t isc, uint16_t mode); int (*inject_airq)(S390FLICState *fs, uint8_t type, uint8_t isc, uint8_t flags); + void (*inject_service)(S390FLICState *fs, uint32_t parm); + void (*inject_io)(S390FLICState *fs, uint16_t subchannel_id, + uint16_t subchannel_nr, uint32_t io_int_parm, + uint32_t io_int_word); + void (*inject_crw_mchk)(S390FLICState *fs); } S390FLICStateClass; #define TYPE_KVM_S390_FLIC "s390-flic-kvm" diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 85f4e6b758..9df851b1e8 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -740,7 +740,12 @@ void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen, uintptr_t ra); /* service interrupts are floating therefore we must not pass an cpustate */ void s390_sclp_extint(uint32_t parm); - +/* FIXME: remove once we have proper floating interrupts in TCG */ +void cpu_inject_service(S390CPU *cpu, uint32_t param); +void cpu_inject_crw_mchk(S390CPU *cpu); +void cpu_inject_io(S390CPU *cpu, uint16_t subchannel_id, + uint16_t subchannel_number, uint32_t io_int_parm, + uint32_t io_int_word); /* mmu_helper.c */ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c index 380222b394..9fce176763 100644 --- a/target/s390x/interrupt.c +++ b/target/s390x/interrupt.c @@ -15,6 +15,9 @@ #include "exec/exec-all.h" #include "sysemu/kvm.h" #include "hw/s390x/ioinst.h" +#if !defined(CONFIG_USER_ONLY) +#include "hw/s390x/s390_flic.h" +#endif /* Ensure to exit the TB after this call! */ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen) @@ -55,7 +58,7 @@ void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen, } #if !defined(CONFIG_USER_ONLY) -static void cpu_inject_service(S390CPU *cpu, uint32_t param) +void cpu_inject_service(S390CPU *cpu, uint32_t param) { CPUS390XState *env = &cpu->env; @@ -134,9 +137,9 @@ void cpu_inject_stop(S390CPU *cpu) cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); } -static void cpu_inject_io(S390CPU *cpu, uint16_t subchannel_id, - uint16_t subchannel_number, - uint32_t io_int_parm, uint32_t io_int_word) +void cpu_inject_io(S390CPU *cpu, uint16_t subchannel_id, + uint16_t subchannel_number, uint32_t io_int_parm, + uint32_t io_int_word) { CPUS390XState *env = &cpu->env; int isc = IO_INT_WORD_ISC(io_int_word); @@ -158,7 +161,7 @@ static void cpu_inject_io(S390CPU *cpu, uint16_t subchannel_id, cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); } -static void cpu_inject_crw_mchk(S390CPU *cpu) +void cpu_inject_crw_mchk(S390CPU *cpu) { CPUS390XState *env = &cpu->env; @@ -173,38 +176,27 @@ static void cpu_inject_crw_mchk(S390CPU *cpu) */ void s390_sclp_extint(uint32_t parm) { - if (kvm_enabled()) { - kvm_s390_service_interrupt(parm); - } else { - S390CPU *dummy_cpu = s390_cpu_addr2state(0); + S390FLICState *fs = s390_get_flic(); + S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(OBJECT(fs)); - cpu_inject_service(dummy_cpu, parm); - } + fsc->inject_service(fs, parm); } void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, uint32_t io_int_parm, uint32_t io_int_word) { - if (kvm_enabled()) { - kvm_s390_io_interrupt(subchannel_id, subchannel_nr, io_int_parm, - io_int_word); - } else { - S390CPU *dummy_cpu = s390_cpu_addr2state(0); + S390FLICState *fs = s390_get_flic(); + S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(OBJECT(fs)); - cpu_inject_io(dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, - io_int_word); - } + fsc->inject_io(fs, subchannel_id, subchannel_nr, io_int_parm, io_int_word); } void s390_crw_mchk(void) { - if (kvm_enabled()) { - kvm_s390_crw_mchk(); - } else { - S390CPU *dummy_cpu = s390_cpu_addr2state(0); + S390FLICState *fs = s390_get_flic(); + S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(OBJECT(fs)); - cpu_inject_crw_mchk(dummy_cpu); - } + fsc->inject_crw_mchk(fs); } bool s390_cpu_has_mcck_int(S390CPU *cpu) diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c index 6bae3e99d3..8cdcf83845 100644 --- a/target/s390x/kvm-stub.c +++ b/target/s390x/kvm-stub.c @@ -12,10 +12,6 @@ #include "cpu.h" #include "kvm_s390x.h" -void kvm_s390_service_interrupt(uint32_t parm) -{ -} - void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) { } @@ -30,15 +26,6 @@ void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code) { } -void kvm_s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, - uint32_t io_int_parm, uint32_t io_int_word) -{ -} - -void kvm_s390_crw_mchk(void) -{ -} - int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) { return -ENOSYS; diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 9b8b59f2a2..aeec1125e1 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -1025,7 +1025,7 @@ void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq) inject_vcpu_irq_legacy(cs, irq); } -static void __kvm_s390_floating_interrupt(struct kvm_s390_irq *irq) +void kvm_s390_floating_interrupt_legacy(struct kvm_s390_irq *irq) { struct kvm_s390_interrupt kvmint = {}; int r; @@ -1043,33 +1043,6 @@ static void __kvm_s390_floating_interrupt(struct kvm_s390_irq *irq) } } -void kvm_s390_floating_interrupt(struct kvm_s390_irq *irq) -{ - static bool use_flic = true; - int r; - - if (use_flic) { - r = kvm_s390_inject_flic(irq); - if (r == -ENOSYS) { - use_flic = false; - } - if (!r) { - return; - } - } - __kvm_s390_floating_interrupt(irq); -} - -void kvm_s390_service_interrupt(uint32_t parm) -{ - struct kvm_s390_irq irq = { - .type = KVM_S390_INT_SERVICE, - .u.ext.ext_params = parm, - }; - - kvm_s390_floating_interrupt(&irq); -} - void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code) { struct kvm_s390_irq irq = { @@ -1681,10 +1654,10 @@ static int handle_tsch(S390CPU *cpu) * If an I/O interrupt had been dequeued, we have to reinject it. */ if (run->s390_tsch.dequeued) { - kvm_s390_io_interrupt(run->s390_tsch.subchannel_id, - run->s390_tsch.subchannel_nr, - run->s390_tsch.io_int_parm, - run->s390_tsch.io_int_word); + s390_io_interrupt(run->s390_tsch.subchannel_id, + run->s390_tsch.subchannel_nr, + run->s390_tsch.io_int_parm, + run->s390_tsch.io_int_word); } ret = 0; } @@ -1831,37 +1804,6 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cpu) return true; } -void kvm_s390_io_interrupt(uint16_t subchannel_id, - uint16_t subchannel_nr, uint32_t io_int_parm, - uint32_t io_int_word) -{ - struct kvm_s390_irq irq = { - .u.io.subchannel_id = subchannel_id, - .u.io.subchannel_nr = subchannel_nr, - .u.io.io_int_parm = io_int_parm, - .u.io.io_int_word = io_int_word, - }; - - if (io_int_word & IO_INT_WORD_AI) { - irq.type = KVM_S390_INT_IO(1, 0, 0, 0); - } else { - irq.type = KVM_S390_INT_IO(0, (subchannel_id & 0xff00) >> 8, - (subchannel_id & 0x0006), - subchannel_nr); - } - kvm_s390_floating_interrupt(&irq); -} - -void kvm_s390_crw_mchk(void) -{ - struct kvm_s390_irq irq = { - .type = KVM_S390_MCHK, - .u.mchk.cr14 = CR14_CHANNEL_REPORT_SC, - .u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP, - }; - kvm_s390_floating_interrupt(&irq); -} - void kvm_s390_enable_css_support(S390CPU *cpu) { int r; diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h index 79b35946f3..7a3b862eea 100644 --- a/target/s390x/kvm_s390x.h +++ b/target/s390x/kvm_s390x.h @@ -12,17 +12,12 @@ struct kvm_s390_irq; -void kvm_s390_floating_interrupt(struct kvm_s390_irq *irq); -void kvm_s390_service_interrupt(uint32_t parm); +void kvm_s390_floating_interrupt_legacy(struct kvm_s390_irq *irq); void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq); void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code); int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, int len, bool is_write); void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code); -void kvm_s390_io_interrupt(uint16_t subchannel_id, - uint16_t subchannel_nr, uint32_t io_int_parm, - uint32_t io_int_word); -void kvm_s390_crw_mchk(void); int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state); void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu); int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu); @@ -44,7 +39,4 @@ void kvm_s390_crypto_reset(void); void kvm_s390_restart_interrupt(S390CPU *cpu); void kvm_s390_stop_interrupt(S390CPU *cpu); -/* implemented outside of target/s390x/ */ -int kvm_s390_inject_flic(struct kvm_s390_irq *irq); - #endif /* KVM_S390X_H */
Let the flic device handle it internally. This will allow us to later on store floating interrupts in the flic for the TCG case. This now also simplifies kvm.c. All that's left is the fallback interface for floating interrupts, which is no triggered directly via the flic in case anything goes wrong. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/intc/s390_flic.c | 31 ++++++++++++++++++++ hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- include/hw/s390x/s390_flic.h | 5 ++++ target/s390x/cpu.h | 7 ++++- target/s390x/interrupt.c | 42 +++++++++++---------------- target/s390x/kvm-stub.c | 13 --------- target/s390x/kvm.c | 68 ++++---------------------------------------- target/s390x/kvm_s390x.h | 10 +------ 8 files changed, 123 insertions(+), 116 deletions(-)