This patch set implements FEAT_NMI and FEAT_GICv3_NMI for ARMv8. These
introduce support for a new category of interrupts in the architecture
which we can use to provide NMI like functionality.

There are two modes for using this FEAT_NMI. When PSTATE.ALLINT or
PSTATE.SP & SCTLR_ELx.SCTLR_SPINTMASK is set, any entry to ELx causes all
interrupts including those with superpriority to be masked on entry to ELn
until the mask is explicitly removed by software or hardware. PSTATE.ALLINT
can be managed by software using the new register control ALLINT.ALLINT.
Independent controls are provided for this feature at each EL, usage at EL1
should not disrupt EL2 or EL3.

I have tested it with the following Linux patches which try to support
FEAT_NMI in Linux kernel:

	https://lore.kernel.org/linux-arm-kernel/Y4sH5qX5bK9xfEBp@lpieralisi/T/#mb4ba4a2c045bf72c10c2202c1dd1b82d3240dc88

In the test, SGI, PPI and SPI interrupts can all be set to have super priority
to be converted to a hardware NMI interrupt. The SGI is tested with kernel
IPI as NMI framework, softlockup, hardlockup and kgdb test cases, and the PPI
interrupt is tested with "perf top" command with hardware NMI enabled, and
the SPI interrupt is tested with a custom test module, in which NMI interrupts
can be received and sent normally.

And the Virtual NMI(VNMI) SGI, PPI and SPI interrupts has also been tested in
nested QEMU Virtual Machine with host "virtualization=true". The SGI VNMI is
tested by accessing GICR_INMIR0 and GICR_ISPENDR0 with devmem command, as well
as hardlockup and kgdb testcases. The PPI VNMI is tested by accessing
GICR_INMIR0 and GICR_ISPENDR0 with devmem command, as well as "perf top"
command with hardware NMI enabled, which works well. The SPI VNMI is tested
with a custom test module, in which SPI VNMI can be sent from the GIC and
received normally.

         +-------------------------------------------------+
         |               Distributor                       |
         +-------------------------------------------------+
             SPI |  NMI                        |  NMI
                \/                            \/
            +--------+                     +-------+
            | Redist |                     | Redist|
            +--------+                     +-------+
            SGI  | NMI                     PPI | NMI
                \/                            \/
          +-------------+             +---------------+
          |CPU Interface|   ...       | CPU Interface |
          +-------------+             +---------------+
               | NMI                         | NMI
              \/                            \/
            +-----+                       +-----+
            |  PE |                       |  PE |
            +-----+                       +-----+

Changes in v13:
- Handle PSTATE.ALLINT the same way as PSTATE.DAIF in the illegal_return
  exit path.
- Adjust "hw/arm/virt: Wire NMI and VINMI irq lines from GIC to CPU" to after
  "hw/intc/arm_gicv3: Add external IRQ lines for NMI" to fix the unexpected
  error with patchseries at this point.
- Only connect NMI irq lines from GIC to CPU if vms->gic_version is not
  VIRT_GIC_VERSION_2 to fix the gic-version=2 unexpected error.
- Enforce RES0 bit in NMI field when FEAT_GICv3_NMI is not implemented for
  ICH_LR<n>_EL2.
- Swap the order of the "irq" and "prio" args in gicv3_get_priority() to make
  input before output.
- Check tcg_enabled() for gicv3_nmi_present().
- Update the comment for gicv3_nmi_present().
- Update the commit message.
- Add Reviewed-by.
- Add Suggested-by.

Changes in v12:
- pPriority<63> = ICC_AP1R_EL1NS<63> if HaveNMIExt() and HaveEL(EL3) and
  (IsNonSecure(), fix the wrong writing.
- Do not check nmi_support repetitively in icc_hppi_can_preempt(),
  and icc_activate_irq, ich_highest_active_virt_prio(), hppvi_index(),
  icv_hppi_can_preempt(), icv_rpr_read() and icv_activate_irq(),
  gicv3_cpuif_virt_irq_fiq_update().
- Check hppi.nmi after check icc_hppi_can_preempt() for icc_iar1_read() and
  icc_nmiar1_read().
- When NMI is 1, the virtual interrupt's priority is 0x0.
- Make the thisnmi logic more concisely in hppvi_index().
- Use is_nmi to simplify code and check is_nmi before comparing vpmr.
- Also check sctlrx.NMI in icv_iar_read().
- Check icv_hppi_can_preempt() for icv_nmiar1_read().
- Check ICH_LR_EL2.NMI after check icv_hppi_can_preempt() as icv_iar_read()
  do it in icv_nmiar1_read().
- Correct thisnmi to bool in icv_eoir_write().
- Check thisnmi and nmi both true instead of identical in icv_eoir_write().
- nmi_needed -> gicv3_cpu_nmi_needed, needed_nmi -> gicv3_nmi_needed.
- Correct the comment style in arm_cpu_initfn() and icv_nmiar1_read().
- Fix the typo, "prioirty" -> "priority".
- Remove the redundant blank line.
- Update the subject and commit message, hppi.superprio -> hppi.nmi,
  super priority -> non-maskable property.
- Add Reviewed-by.

Changes in v11:
- Put vmstate_gicv3_cpu_nmi and vmstate_gicv3_gicd_nmi into existing list.
- Remove the excess != 0.
- Handle NMI priority in icc_highest_active_prio() and handle NMI RPR in
  icc_rpr_read() separately.
- Only set NMI bit for a NMI and and ordinary priority bit for a non-NMI in
  icc_activate_irq().
- Only clear APR bit for AP1R0 in icc_drop_prio().
- Check special INTID_* in callers instead of passing two extra boolean args
  for ack functions.
- Handle NMI in icc_hppi_can_preempt() and icc_highest_active_group().
- Also check icc_hppi_can_preempt() for icc_nmiar1_read().
- Deal with NMI in the callers instead of ich_highest_active_virt_prio().
- Set either NMI or a group-priority bit, not both.
- Only set AP NMI bits in the 0 reg.
- Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
- Add Reviewed-by.

Changes in v10:
- Correct the exception_target_el(env) to 2 in msr_set_allint_el1 helper,
  since it is a hypervisor trap from EL1 to EL2.
- In arm_cpu_exec_interrupt(), if SCTLR_ELx.NMI is 0, NMI -> IRQ,
  VINMI -> VIRQ, VFNMI -> VFIQ.
- Make arm_cpu_update_virq() and arm_cpu_update_vfiq() check that it is not a
  VINMI/VFNMI, so only set 1 bit in interrupt_request, not 2.
- Adjust "hw/intc: Enable FEAT_GICv3_NMI Feature" to before "add irq
  non-maskable property".
- superprio -> nmi, gicr_isuperprio -> gicr_inmir0, is_nmi -> nmi,
  is_hppi -> hppi, has_superprio -> nmi, superpriority -> non-maskable property.
- Save NMI state in vmstate_gicv3_cpu and vmstate_gicv3.
- Exchange the order of nmi and hppi parameters.
- Handle APR and RPR NMI bits, rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
- Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
  ich_highest_active_virt_prio()
- Update the commit message.

Changes in v9:
- Move nmi_reginfo and related functions inside an existing ifdef
  TARGET_AARCH64 to solve the --target-list=aarch64-softmmu,arm-softmmu
  compilation problem.
- Check 'isread' when writing to ALLINT.
- Update the GPIOs passed in the arm_cpu_kvm_set_irq, and update the comment.
- Definitely not merge VINMI and VFNMI into EXCP_VNMI.
- ARM_CPU_VNMI -> ARM_CPU_VINMI, CPU_INTERRUPT_VNMI -> CPU_INTERRUPT_VINMI.
- Update VINMI and VFNMI when writing HCR_EL2 or HCRX_EL2.
- Update the commit subject and message, VNMI -> VINMI.
- Handle CPSR_F and ISR_FS according to CPU_INTERRUPT_VFNMI instead of
  CPU_INTERRUPT_VFIQ and HCRX_EL2.VFNMI.
- Not check SCTLR_NMI in arm_cpu_do_interrupt_aarch64().
- Correct the INTID_NMI logic.
- Declare cpu variable to reuse latter.

Changes in v8:
- Fix the rcu stall after sending a VNMI in qemu VM.
- Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
- Test the VNMI interrupt.
- Update the commit message.
- Add Reviewed-by.

Changes in v7:
- env->cp15.hcrx_el2 -> arm_hcrx_el2_eff().
- Reorder the irqbetter() code for clarity.
- Eliminate the has_superprio local variable for gicv3_get_priority().
- false -> cs->hpplpi.superprio in gicv3_redist_update_noirqset().
- 0x0 -> false in arm_gicv3_common_reset_hold().
- Clear superprio in several places for hppi, hpplpi and hppvlpi.
- Add Reviewed-by.

Changes in v6:
- Fix DISAS_TOO_MANY to DISAS_UPDATE_EXIT for ALLINT MSR (immediate).
- Verify that HCR_EL2.VF is set before checking VFNMI.
- env->cp15.hcr_el2 -> arm_hcr_el2_eff().
- env->cp15.hcrx_el2 -> arm_hcrx_el2_eff().
- Not combine VFNMI with CPU_INTERRUPT_VNMI.
- Implement icv_nmiar1_read().
- Put the "extract superprio info" code into gicv3_get_priority().
- Update the comment in irqbetter().
- Reset the cs->hppi.superprio to 0x0.
- Set hppi.superprio to false for LPI.
- Add Reviewed-by.

Changes in v5:
- Remove the comment for ALLINT in cpu.h.
- Merge allint_check() to msr_i_allint to clear the ALLINT MSR (immediate)
  implementation.
- Rename msr_i_allint() to msr_set_allint_el1() to make it clearer.
- Drop the & 1 in trans_MSR_i_ALLINT().
- Add Reviewed-by.

Changes in v4:
- Handle VNMI within the CPU and the GIC.
- Keep PSTATE.ALLINT in env->pstate but not env->allint.
- Fix the ALLINT MSR (immediate) decodetree implementation.
- Accept NMI unconditionally for arm_cpu_has_work() but add comment.
- Improve nmi mask in arm_excp_unmasked().
- Make the GICR_INMIR0 and GICD_INMIR implementation more clearer.
- Improve ICC_NMIAR1_EL1 implementation
- Extract gicv3_get_priority() to avoid priority code repetition.
- Add Reviewed-by.

Changes in v3:
- Remove the FIQ NMI.
- Adjust the patches Sequence.
- Reomve the patch "Set pstate.ALLINT in arm_cpu_reset_hold".
- Check whether support FEAT_NMI and FEAT_GICv3 for FEAT_GICv3_NMI.
- With CPU_INTERRUPT_NMI, both CPSR_I and ISR_IS must be set.
- Not include NMI logic when FEAT_NMI or SCTLR_ELx.NMI not enabled.
- Refator nmi mask in arm_excp_unmasked().
- Add VNMI definitions, add HCRX_VINMI and HCRX_VFNMI support in HCRX_EL2.
- Add Reviewed-by and Acked-by.
- Update the commit message.

Changes in v2:
- Break up the patches so that each one does only one thing.
- Remove the command line option and just implement it in "max" cpu.

Jinjie Ruan (24):
  target/arm: Handle HCR_EL2 accesses for bits introduced with FEAT_NMI
  target/arm: Add PSTATE.ALLINT
  target/arm: Add support for FEAT_NMI, Non-maskable Interrupt
  target/arm: Implement ALLINT MSR (immediate)
  target/arm: Support MSR access to ALLINT
  target/arm: Add support for Non-maskable Interrupt
  target/arm: Add support for NMI in arm_phys_excp_target_el()
  target/arm: Handle IS/FS in ISR_EL1 for NMI, VINMI and VFNMI
  target/arm: Handle PSTATE.ALLINT on taking an exception
  hw/intc/arm_gicv3: Add external IRQ lines for NMI
  hw/arm/virt: Wire NMI and VINMI irq lines from GIC to CPU
  target/arm: Handle NMI in arm_cpu_do_interrupt_aarch64()
  hw/intc/arm_gicv3: Add has-nmi property to GICv3 device
  hw/intc/arm_gicv3_kvm: Not set has-nmi=true for the KVM GICv3
  hw/intc/arm_gicv3: Add irq non-maskable property
  hw/intc/arm_gicv3_redist: Implement GICR_INMIR0
  hw/intc/arm_gicv3: Implement GICD_INMIR
  hw/intc/arm_gicv3: Add NMI handling CPU interface registers
  hw/intc/arm_gicv3: Handle icv_nmiar1_read() for icc_nmiar1_read()
  hw/intc/arm_gicv3: Implement NMI interrupt priority
  hw/intc/arm_gicv3: Report the NMI interrupt in gicv3_cpuif_update()
  hw/intc/arm_gicv3: Report the VINMI interrupt
  target/arm: Add FEAT_NMI to max
  hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC

 docs/system/arm/emulation.rst      |   1 +
 hw/arm/virt.c                      |  27 ++-
 hw/intc/arm_gicv3.c                |  67 +++++++-
 hw/intc/arm_gicv3_common.c         |  48 ++++++
 hw/intc/arm_gicv3_cpuif.c          | 258 +++++++++++++++++++++++++++--
 hw/intc/arm_gicv3_dist.c           |  36 ++++
 hw/intc/arm_gicv3_kvm.c            |   5 +
 hw/intc/arm_gicv3_redist.c         |  22 +++
 hw/intc/gicv3_internal.h           |  13 ++
 hw/intc/trace-events               |   2 +
 include/hw/intc/arm_gic_common.h   |   2 +
 include/hw/intc/arm_gicv3_common.h |   7 +
 target/arm/cpu-features.h          |   5 +
 target/arm/cpu-qom.h               |   5 +-
 target/arm/cpu.c                   | 147 ++++++++++++++--
 target/arm/cpu.h                   |   9 +
 target/arm/helper.c                | 101 ++++++++++-
 target/arm/internals.h             |  21 +++
 target/arm/tcg/a64.decode          |   1 +
 target/arm/tcg/cpu64.c             |   1 +
 target/arm/tcg/helper-a64.c        |  16 +-
 target/arm/tcg/helper-a64.h        |   1 +
 target/arm/tcg/translate-a64.c     |  19 +++
 23 files changed, 767 insertions(+), 47 deletions(-)

-- 
2.34.1

Ping.

On 2024/4/7 16:17, Jinjie Ruan wrote:
> This patch set implements FEAT_NMI and FEAT_GICv3_NMI for ARMv8. These
> introduce support for a new category of interrupts in the architecture
> which we can use to provide NMI like functionality.
> 
> There are two modes for using this FEAT_NMI. When PSTATE.ALLINT or
> PSTATE.SP & SCTLR_ELx.SCTLR_SPINTMASK is set, any entry to ELx causes all
> interrupts including those with superpriority to be masked on entry to ELn
> until the mask is explicitly removed by software or hardware. PSTATE.ALLINT
> can be managed by software using the new register control ALLINT.ALLINT.
> Independent controls are provided for this feature at each EL, usage at EL1
> should not disrupt EL2 or EL3.
> 
> I have tested it with the following Linux patches which try to support
> FEAT_NMI in Linux kernel:
> 
> 	https://lore.kernel.org/linux-arm-kernel/Y4sH5qX5bK9xfEBp@lpieralisi/T/#mb4ba4a2c045bf72c10c2202c1dd1b82d3240dc88
> 
> In the test, SGI, PPI and SPI interrupts can all be set to have super priority
> to be converted to a hardware NMI interrupt. The SGI is tested with kernel
> IPI as NMI framework, softlockup, hardlockup and kgdb test cases, and the PPI
> interrupt is tested with "perf top" command with hardware NMI enabled, and
> the SPI interrupt is tested with a custom test module, in which NMI interrupts
> can be received and sent normally.
> 
> And the Virtual NMI(VNMI) SGI, PPI and SPI interrupts has also been tested in
> nested QEMU Virtual Machine with host "virtualization=true". The SGI VNMI is
> tested by accessing GICR_INMIR0 and GICR_ISPENDR0 with devmem command, as well
> as hardlockup and kgdb testcases. The PPI VNMI is tested by accessing
> GICR_INMIR0 and GICR_ISPENDR0 with devmem command, as well as "perf top"
> command with hardware NMI enabled, which works well. The SPI VNMI is tested
> with a custom test module, in which SPI VNMI can be sent from the GIC and
> received normally.
> 
>          +-------------------------------------------------+
>          |               Distributor                       |
>          +-------------------------------------------------+
>              SPI |  NMI                        |  NMI
>                 \/                            \/
>             +--------+                     +-------+
>             | Redist |                     | Redist|
>             +--------+                     +-------+
>             SGI  | NMI                     PPI | NMI
>                 \/                            \/
>           +-------------+             +---------------+
>           |CPU Interface|   ...       | CPU Interface |
>           +-------------+             +---------------+
>                | NMI                         | NMI
>               \/                            \/
>             +-----+                       +-----+
>             |  PE |                       |  PE |
>             +-----+                       +-----+
> 
> Changes in v13:
> - Handle PSTATE.ALLINT the same way as PSTATE.DAIF in the illegal_return
>   exit path.
> - Adjust "hw/arm/virt: Wire NMI and VINMI irq lines from GIC to CPU" to after
>   "hw/intc/arm_gicv3: Add external IRQ lines for NMI" to fix the unexpected
>   error with patchseries at this point.
> - Only connect NMI irq lines from GIC to CPU if vms->gic_version is not
>   VIRT_GIC_VERSION_2 to fix the gic-version=2 unexpected error.
> - Enforce RES0 bit in NMI field when FEAT_GICv3_NMI is not implemented for
>   ICH_LR<n>_EL2.
> - Swap the order of the "irq" and "prio" args in gicv3_get_priority() to make
>   input before output.
> - Check tcg_enabled() for gicv3_nmi_present().
> - Update the comment for gicv3_nmi_present().
> - Update the commit message.
> - Add Reviewed-by.
> - Add Suggested-by.
> 
> Changes in v12:
> - pPriority<63> = ICC_AP1R_EL1NS<63> if HaveNMIExt() and HaveEL(EL3) and
>   (IsNonSecure(), fix the wrong writing.
> - Do not check nmi_support repetitively in icc_hppi_can_preempt(),
>   and icc_activate_irq, ich_highest_active_virt_prio(), hppvi_index(),
>   icv_hppi_can_preempt(), icv_rpr_read() and icv_activate_irq(),
>   gicv3_cpuif_virt_irq_fiq_update().
> - Check hppi.nmi after check icc_hppi_can_preempt() for icc_iar1_read() and
>   icc_nmiar1_read().
> - When NMI is 1, the virtual interrupt's priority is 0x0.
> - Make the thisnmi logic more concisely in hppvi_index().
> - Use is_nmi to simplify code and check is_nmi before comparing vpmr.
> - Also check sctlrx.NMI in icv_iar_read().
> - Check icv_hppi_can_preempt() for icv_nmiar1_read().
> - Check ICH_LR_EL2.NMI after check icv_hppi_can_preempt() as icv_iar_read()
>   do it in icv_nmiar1_read().
> - Correct thisnmi to bool in icv_eoir_write().
> - Check thisnmi and nmi both true instead of identical in icv_eoir_write().
> - nmi_needed -> gicv3_cpu_nmi_needed, needed_nmi -> gicv3_nmi_needed.
> - Correct the comment style in arm_cpu_initfn() and icv_nmiar1_read().
> - Fix the typo, "prioirty" -> "priority".
> - Remove the redundant blank line.
> - Update the subject and commit message, hppi.superprio -> hppi.nmi,
>   super priority -> non-maskable property.
> - Add Reviewed-by.
> 
> Changes in v11:
> - Put vmstate_gicv3_cpu_nmi and vmstate_gicv3_gicd_nmi into existing list.
> - Remove the excess != 0.
> - Handle NMI priority in icc_highest_active_prio() and handle NMI RPR in
>   icc_rpr_read() separately.
> - Only set NMI bit for a NMI and and ordinary priority bit for a non-NMI in
>   icc_activate_irq().
> - Only clear APR bit for AP1R0 in icc_drop_prio().
> - Check special INTID_* in callers instead of passing two extra boolean args
>   for ack functions.
> - Handle NMI in icc_hppi_can_preempt() and icc_highest_active_group().
> - Also check icc_hppi_can_preempt() for icc_nmiar1_read().
> - Deal with NMI in the callers instead of ich_highest_active_virt_prio().
> - Set either NMI or a group-priority bit, not both.
> - Only set AP NMI bits in the 0 reg.
> - Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
> - Add Reviewed-by.
> 
> Changes in v10:
> - Correct the exception_target_el(env) to 2 in msr_set_allint_el1 helper,
>   since it is a hypervisor trap from EL1 to EL2.
> - In arm_cpu_exec_interrupt(), if SCTLR_ELx.NMI is 0, NMI -> IRQ,
>   VINMI -> VIRQ, VFNMI -> VFIQ.
> - Make arm_cpu_update_virq() and arm_cpu_update_vfiq() check that it is not a
>   VINMI/VFNMI, so only set 1 bit in interrupt_request, not 2.
> - Adjust "hw/intc: Enable FEAT_GICv3_NMI Feature" to before "add irq
>   non-maskable property".
> - superprio -> nmi, gicr_isuperprio -> gicr_inmir0, is_nmi -> nmi,
>   is_hppi -> hppi, has_superprio -> nmi, superpriority -> non-maskable property.
> - Save NMI state in vmstate_gicv3_cpu and vmstate_gicv3.
> - Exchange the order of nmi and hppi parameters.
> - Handle APR and RPR NMI bits, rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
> - Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
>   ich_highest_active_virt_prio()
> - Update the commit message.
> 
> Changes in v9:
> - Move nmi_reginfo and related functions inside an existing ifdef
>   TARGET_AARCH64 to solve the --target-list=aarch64-softmmu,arm-softmmu
>   compilation problem.
> - Check 'isread' when writing to ALLINT.
> - Update the GPIOs passed in the arm_cpu_kvm_set_irq, and update the comment.
> - Definitely not merge VINMI and VFNMI into EXCP_VNMI.
> - ARM_CPU_VNMI -> ARM_CPU_VINMI, CPU_INTERRUPT_VNMI -> CPU_INTERRUPT_VINMI.
> - Update VINMI and VFNMI when writing HCR_EL2 or HCRX_EL2.
> - Update the commit subject and message, VNMI -> VINMI.
> - Handle CPSR_F and ISR_FS according to CPU_INTERRUPT_VFNMI instead of
>   CPU_INTERRUPT_VFIQ and HCRX_EL2.VFNMI.
> - Not check SCTLR_NMI in arm_cpu_do_interrupt_aarch64().
> - Correct the INTID_NMI logic.
> - Declare cpu variable to reuse latter.
> 
> Changes in v8:
> - Fix the rcu stall after sending a VNMI in qemu VM.
> - Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
> - Test the VNMI interrupt.
> - Update the commit message.
> - Add Reviewed-by.
> 
> Changes in v7:
> - env->cp15.hcrx_el2 -> arm_hcrx_el2_eff().
> - Reorder the irqbetter() code for clarity.
> - Eliminate the has_superprio local variable for gicv3_get_priority().
> - false -> cs->hpplpi.superprio in gicv3_redist_update_noirqset().
> - 0x0 -> false in arm_gicv3_common_reset_hold().
> - Clear superprio in several places for hppi, hpplpi and hppvlpi.
> - Add Reviewed-by.
> 
> Changes in v6:
> - Fix DISAS_TOO_MANY to DISAS_UPDATE_EXIT for ALLINT MSR (immediate).
> - Verify that HCR_EL2.VF is set before checking VFNMI.
> - env->cp15.hcr_el2 -> arm_hcr_el2_eff().
> - env->cp15.hcrx_el2 -> arm_hcrx_el2_eff().
> - Not combine VFNMI with CPU_INTERRUPT_VNMI.
> - Implement icv_nmiar1_read().
> - Put the "extract superprio info" code into gicv3_get_priority().
> - Update the comment in irqbetter().
> - Reset the cs->hppi.superprio to 0x0.
> - Set hppi.superprio to false for LPI.
> - Add Reviewed-by.
> 
> Changes in v5:
> - Remove the comment for ALLINT in cpu.h.
> - Merge allint_check() to msr_i_allint to clear the ALLINT MSR (immediate)
>   implementation.
> - Rename msr_i_allint() to msr_set_allint_el1() to make it clearer.
> - Drop the & 1 in trans_MSR_i_ALLINT().
> - Add Reviewed-by.
> 
> Changes in v4:
> - Handle VNMI within the CPU and the GIC.
> - Keep PSTATE.ALLINT in env->pstate but not env->allint.
> - Fix the ALLINT MSR (immediate) decodetree implementation.
> - Accept NMI unconditionally for arm_cpu_has_work() but add comment.
> - Improve nmi mask in arm_excp_unmasked().
> - Make the GICR_INMIR0 and GICD_INMIR implementation more clearer.
> - Improve ICC_NMIAR1_EL1 implementation
> - Extract gicv3_get_priority() to avoid priority code repetition.
> - Add Reviewed-by.
> 
> Changes in v3:
> - Remove the FIQ NMI.
> - Adjust the patches Sequence.
> - Reomve the patch "Set pstate.ALLINT in arm_cpu_reset_hold".
> - Check whether support FEAT_NMI and FEAT_GICv3 for FEAT_GICv3_NMI.
> - With CPU_INTERRUPT_NMI, both CPSR_I and ISR_IS must be set.
> - Not include NMI logic when FEAT_NMI or SCTLR_ELx.NMI not enabled.
> - Refator nmi mask in arm_excp_unmasked().
> - Add VNMI definitions, add HCRX_VINMI and HCRX_VFNMI support in HCRX_EL2.
> - Add Reviewed-by and Acked-by.
> - Update the commit message.
> 
> Changes in v2:
> - Break up the patches so that each one does only one thing.
> - Remove the command line option and just implement it in "max" cpu.
> 
> Jinjie Ruan (24):
>   target/arm: Handle HCR_EL2 accesses for bits introduced with FEAT_NMI
>   target/arm: Add PSTATE.ALLINT
>   target/arm: Add support for FEAT_NMI, Non-maskable Interrupt
>   target/arm: Implement ALLINT MSR (immediate)
>   target/arm: Support MSR access to ALLINT
>   target/arm: Add support for Non-maskable Interrupt
>   target/arm: Add support for NMI in arm_phys_excp_target_el()
>   target/arm: Handle IS/FS in ISR_EL1 for NMI, VINMI and VFNMI
>   target/arm: Handle PSTATE.ALLINT on taking an exception
>   hw/intc/arm_gicv3: Add external IRQ lines for NMI
>   hw/arm/virt: Wire NMI and VINMI irq lines from GIC to CPU
>   target/arm: Handle NMI in arm_cpu_do_interrupt_aarch64()
>   hw/intc/arm_gicv3: Add has-nmi property to GICv3 device
>   hw/intc/arm_gicv3_kvm: Not set has-nmi=true for the KVM GICv3
>   hw/intc/arm_gicv3: Add irq non-maskable property
>   hw/intc/arm_gicv3_redist: Implement GICR_INMIR0
>   hw/intc/arm_gicv3: Implement GICD_INMIR
>   hw/intc/arm_gicv3: Add NMI handling CPU interface registers
>   hw/intc/arm_gicv3: Handle icv_nmiar1_read() for icc_nmiar1_read()
>   hw/intc/arm_gicv3: Implement NMI interrupt priority
>   hw/intc/arm_gicv3: Report the NMI interrupt in gicv3_cpuif_update()
>   hw/intc/arm_gicv3: Report the VINMI interrupt
>   target/arm: Add FEAT_NMI to max
>   hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC
> 
>  docs/system/arm/emulation.rst      |   1 +
>  hw/arm/virt.c                      |  27 ++-
>  hw/intc/arm_gicv3.c                |  67 +++++++-
>  hw/intc/arm_gicv3_common.c         |  48 ++++++
>  hw/intc/arm_gicv3_cpuif.c          | 258 +++++++++++++++++++++++++++--
>  hw/intc/arm_gicv3_dist.c           |  36 ++++
>  hw/intc/arm_gicv3_kvm.c            |   5 +
>  hw/intc/arm_gicv3_redist.c         |  22 +++
>  hw/intc/gicv3_internal.h           |  13 ++
>  hw/intc/trace-events               |   2 +
>  include/hw/intc/arm_gic_common.h   |   2 +
>  include/hw/intc/arm_gicv3_common.h |   7 +
>  target/arm/cpu-features.h          |   5 +
>  target/arm/cpu-qom.h               |   5 +-
>  target/arm/cpu.c                   | 147 ++++++++++++++--
>  target/arm/cpu.h                   |   9 +
>  target/arm/helper.c                | 101 ++++++++++-
>  target/arm/internals.h             |  21 +++
>  target/arm/tcg/a64.decode          |   1 +
>  target/arm/tcg/cpu64.c             |   1 +
>  target/arm/tcg/helper-a64.c        |  16 +-
>  target/arm/tcg/helper-a64.h        |   1 +
>  target/arm/tcg/translate-a64.c     |  19 +++
>  23 files changed, 767 insertions(+), 47 deletions(-)
>

On Wed, 10 Apr 2024 at 07:19, Jinjie Ruan via <qemu-arm@nongnu.org> wrote:
>
> Ping.

As I said in my reply on the previous version, we're in
freeze at the moment, so this patchset is not going anywhere
until 9.0 releases. I think it's in shape to apply after that.

thanks
-- PMM

On 2024/4/10 20:58, Peter Maydell wrote:
> On Wed, 10 Apr 2024 at 07:19, Jinjie Ruan via <qemu-arm@nongnu.org> wrote:
>>
>> Ping.
> 
> As I said in my reply on the previous version, we're in
> freeze at the moment, so this patchset is not going anywhere
> until 9.0 releases. I think it's in shape to apply after that.

Thank you! I don't know if there's any other minor issues.

> 
> thanks
> -- PMM

On Sun, 7 Apr 2024 at 09:19, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
> This patch set implements FEAT_NMI and FEAT_GICv3_NMI for ARMv8. These
> introduce support for a new category of interrupts in the architecture
> which we can use to provide NMI like functionality.

I had one last loose end I wanted to tidy up, and I got round
to working through reading the spec about it today. This is
the question of what the "is NMI enabled?" test should be
in the code in arm_gicv3_cpuif.c.

The spec wording isn't always super clear, but there are several
things here:

 * FEAT_NMI : the changes to the CPU proper which implement
   superpriority for IRQ and FIQ, PSTATE.ALLINT, etc etc.
 * FEAT_GICv3_NMI : the changes to the CPU interface for
   GICv3 NMI handling. Any CPU with FEAT_NMI and FEAT_GICv3
   must have this.
 * NMI support in the IRI (Interrupt Routing Infrastructure,
   i.e. all the bits of the GIC that aren't the cpuif; the
   distributor and redistributors). Table 3-1 in the GIC spec
   says that you can have an IRI without NMI support connected
   to a CPU which does have NMI support. This is what the ID
   register bit GICD_TYPER.NMI reports.

At the moment this patchset conflates FEAT_GICv3_NMI and
the NMI support in the IRI. The effect of this is that we
allow a machine model to create a CPU with FEAT_NMI but
without FEAT_GICv3_NMI in the cpuif, and we don't allow
a setup where the CPU and cpuif have NMI support but the
IRI does not. (This will actually happen with this patchset
with the sbsa-ref machine and -cpu max, because we haven't
(yet) made sbsa-ref enable NMI in the GIC device when the
CPU has NMI support.)

For a Linux guest this doesn't make much difference, because
Linux will only enable NMI support if it finds it in both
the IRI and the CPU, but I think it would be better to
get the enable-tests right as these can be awkward to change
after the fact in a backwards-compatible way.

I think this is easy to fix -- we can add a new bool field
GICv3CPUState::nmi_support which we initialize in
gicv3_init_cpuif() if the CPU has FEAT_NMI, and make the
checks in arm_gicv3_cpuif.c check cs->nmi_support instead
of cs->gic->nmi_support. That looks like this squashed into
patch 18:

diff --git a/include/hw/intc/arm_gicv3_common.h
b/include/hw/intc/arm_gicv3_common.h
index 88533749ebb..cd09bee3bc4 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -225,6 +225,13 @@ struct GICv3CPUState {

     /* This is temporary working state, to avoid a malloc in gicv3_update() */
     bool seenbetter;
+
+    /*
+     * Whether the CPU interface has NMI support (FEAT_GICv3_NMI). The
+     * CPU interface may support NMIs even when the GIC proper (what the
+     * spec calls the IRI; the redistributors and distributor) does not.
+     */
+    bool nmi_support;
 };

 /*
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 2457b7bca23..715909d0f7d 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -21,6 +21,7 @@
 #include "hw/irq.h"
 #include "cpu.h"
 #include "target/arm/cpregs.h"
+#include "target/arm/cpu-features.h"
 #include "sysemu/tcg.h"
 #include "sysemu/qtest.h"

@@ -839,7 +840,7 @@ static int icc_highest_active_prio(GICv3CPUState *cs)
      */
     int i;

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         /*
          * If an NMI is active this takes precedence over anything else
          * for priority purposes; the NMI bit is only in the AP1R0 bit.
@@ -1285,7 +1286,7 @@ static void icc_drop_prio(GICv3CPUState *cs, int grp)
             continue;
         }

-        if (i == 0 && cs->gic->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) {
+        if (i == 0 && cs->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) {
             *papr &= (~ICC_AP1R_EL1_NMI);
             break;
         }
@@ -1324,7 +1325,7 @@ static int icc_highest_active_group(GICv3CPUState *cs)
      */
     int i;

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) {
             return GICV3_G1;
         }
@@ -1787,7 +1788,7 @@ static void icc_ap_write(CPUARMState *env, const
ARMCPRegInfo *ri,
         return;
     }

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         cs->icc_apr[grp][regno] = value & (0xFFFFFFFFU | ICC_AP1R_EL1_NMI);
     } else {
         cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU;
@@ -1901,7 +1902,7 @@ static uint64_t icc_rpr_read(CPUARMState *env,
const ARMCPRegInfo *ri)
         }
     }

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         /* NMI info is reported in the high bits of RPR */
         if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
             if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
@@ -2961,7 +2962,16 @@ void gicv3_init_cpuif(GICv3State *s)
          */
         define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);

-        if (s->nmi_support) {
+        /*
+         * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also
+         * implement FEAT_GICv3_NMI, which is the CPU interface part
+         * of NMI support. This is distinct from whether the GIC proper
+         * (redistributors and distributor) have NMI support. In QEMU
+         * that is a property of the GIC device in s->nmi_support;
+         * cs->nmi_support indicates the CPU interface's support.
+         */
+        if (cpu_isar_feature(aa64_nmi, cpu)) {
+            cs->nmi_support = true;
             define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo);
         }

plus this squashed into patch 19:

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 20a8e1f2fe4..b1f6c16ffef 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -566,7 +566,7 @@ static void icv_ap_write(CPUARMState *env, const
ARMCPRegInfo *ri,

     trace_gicv3_icv_ap_write(ri->crm & 1, regno,
gicv3_redist_affid(cs), value);

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
     } else {
         cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
@@ -1510,7 +1510,7 @@ static int icv_drop_prio(GICv3CPUState *cs, bool *nmi)
             continue;
         }

-        if (i == 0 && cs->gic->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
+        if (i == 0 && cs->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
             *papr1 &= (~ICV_AP1R_EL1_NMI);
             *nmi = true;
             return 0xff;
@@ -2699,7 +2699,7 @@ static void ich_ap_write(CPUARMState *env, const
ARMCPRegInfo *ri,

     trace_gicv3_ich_ap_write(ri->crm & 1, regno,
gicv3_redist_affid(cs), value);

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
     } else {
         cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
@@ -2821,7 +2821,7 @@ static void ich_lr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
     }

     /* Enforce RES0 bit in NMI field when FEAT_GICv3_NMI is not implemented */
-    if (!cs->gic->nmi_support) {
+    if (!cs->nmi_support) {
         value &= ~ICH_LR_EL2_NMI;
     }

The comments and commit message for patch 24 also need tweaking,
because they are written assuming that FEAT_GICv3_NMI means
"NMI support in the GIC proper", not "NMI support in the cpuif".

Since those changes are not too complicated, and I made them
locally anyway since I wanted to confirm that my plan was
workable, my proposal is that I will apply these fixes while
I take this series into target-arm.next for 9.1.

So I've applied this series to target-arm.next with the above
changes (preparatory to doing a pull request tail end of next
week once we release 9.0). Let me know if you'd prefer something
else.

thanks
-- PMM

On 2024/4/19 21:41, Peter Maydell wrote:
> On Sun, 7 Apr 2024 at 09:19, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>> This patch set implements FEAT_NMI and FEAT_GICv3_NMI for ARMv8. These
>> introduce support for a new category of interrupts in the architecture
>> which we can use to provide NMI like functionality.
> 
> I had one last loose end I wanted to tidy up, and I got round
> to working through reading the spec about it today. This is
> the question of what the "is NMI enabled?" test should be
> in the code in arm_gicv3_cpuif.c.
> 
> The spec wording isn't always super clear, but there are several
> things here:
> 
>  * FEAT_NMI : the changes to the CPU proper which implement
>    superpriority for IRQ and FIQ, PSTATE.ALLINT, etc etc.
>  * FEAT_GICv3_NMI : the changes to the CPU interface for
>    GICv3 NMI handling. Any CPU with FEAT_NMI and FEAT_GICv3
>    must have this.
>  * NMI support in the IRI (Interrupt Routing Infrastructure,
>    i.e. all the bits of the GIC that aren't the cpuif; the
>    distributor and redistributors). Table 3-1 in the GIC spec
>    says that you can have an IRI without NMI support connected
>    to a CPU which does have NMI support. This is what the ID
>    register bit GICD_TYPER.NMI reports.

That is right. It seems reasonable for me.

> 
> At the moment this patchset conflates FEAT_GICv3_NMI and
> the NMI support in the IRI. The effect of this is that we
> allow a machine model to create a CPU with FEAT_NMI but
> without FEAT_GICv3_NMI in the cpuif, and we don't allow
> a setup where the CPU and cpuif have NMI support but the
> IRI does not. (This will actually happen with this patchset
> with the sbsa-ref machine and -cpu max, because we haven't
> (yet) made sbsa-ref enable NMI in the GIC device when the
> CPU has NMI support.)
> 
> For a Linux guest this doesn't make much difference, because
> Linux will only enable NMI support if it finds it in both
> the IRI and the CPU, but I think it would be better to
> get the enable-tests right as these can be awkward to change
> after the fact in a backwards-compatible way.
> 
> I think this is easy to fix -- we can add a new bool field
> GICv3CPUState::nmi_support which we initialize in
> gicv3_init_cpuif() if the CPU has FEAT_NMI, and make the
> checks in arm_gicv3_cpuif.c check cs->nmi_support instead
> of cs->gic->nmi_support. That looks like this squashed into
> patch 18:
> 
> diff --git a/include/hw/intc/arm_gicv3_common.h
> b/include/hw/intc/arm_gicv3_common.h
> index 88533749ebb..cd09bee3bc4 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -225,6 +225,13 @@ struct GICv3CPUState {
> 
>      /* This is temporary working state, to avoid a malloc in gicv3_update() */
>      bool seenbetter;
> +
> +    /*
> +     * Whether the CPU interface has NMI support (FEAT_GICv3_NMI). The
> +     * CPU interface may support NMIs even when the GIC proper (what the
> +     * spec calls the IRI; the redistributors and distributor) does not.
> +     */
> +    bool nmi_support;
>  };
> 
>  /*
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 2457b7bca23..715909d0f7d 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -21,6 +21,7 @@
>  #include "hw/irq.h"
>  #include "cpu.h"
>  #include "target/arm/cpregs.h"
> +#include "target/arm/cpu-features.h"
>  #include "sysemu/tcg.h"
>  #include "sysemu/qtest.h"
> 
> @@ -839,7 +840,7 @@ static int icc_highest_active_prio(GICv3CPUState *cs)
>       */
>      int i;
> 
> -    if (cs->gic->nmi_support) {
> +    if (cs->nmi_support) {
>          /*
>           * If an NMI is active this takes precedence over anything else
>           * for priority purposes; the NMI bit is only in the AP1R0 bit.
> @@ -1285,7 +1286,7 @@ static void icc_drop_prio(GICv3CPUState *cs, int grp)
>              continue;
>          }
> 
> -        if (i == 0 && cs->gic->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) {
> +        if (i == 0 && cs->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) {
>              *papr &= (~ICC_AP1R_EL1_NMI);
>              break;
>          }
> @@ -1324,7 +1325,7 @@ static int icc_highest_active_group(GICv3CPUState *cs)
>       */
>      int i;
> 
> -    if (cs->gic->nmi_support) {
> +    if (cs->nmi_support) {
>          if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) {
>              return GICV3_G1;
>          }
> @@ -1787,7 +1788,7 @@ static void icc_ap_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
>          return;
>      }
> 
> -    if (cs->gic->nmi_support) {
> +    if (cs->nmi_support) {
>          cs->icc_apr[grp][regno] = value & (0xFFFFFFFFU | ICC_AP1R_EL1_NMI);
>      } else {
>          cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU;
> @@ -1901,7 +1902,7 @@ static uint64_t icc_rpr_read(CPUARMState *env,
> const ARMCPRegInfo *ri)
>          }
>      }
> 
> -    if (cs->gic->nmi_support) {
> +    if (cs->nmi_support) {
>          /* NMI info is reported in the high bits of RPR */
>          if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
>              if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
> @@ -2961,7 +2962,16 @@ void gicv3_init_cpuif(GICv3State *s)
>           */
>          define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
> 
> -        if (s->nmi_support) {
> +        /*
> +         * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also
> +         * implement FEAT_GICv3_NMI, which is the CPU interface part
> +         * of NMI support. This is distinct from whether the GIC proper
> +         * (redistributors and distributor) have NMI support. In QEMU
> +         * that is a property of the GIC device in s->nmi_support;
> +         * cs->nmi_support indicates the CPU interface's support.
> +         */
> +        if (cpu_isar_feature(aa64_nmi, cpu)) {
> +            cs->nmi_support = true;
>              define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo);
>          }
> 
> plus this squashed into patch 19:
> 
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 20a8e1f2fe4..b1f6c16ffef 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -566,7 +566,7 @@ static void icv_ap_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> 
>      trace_gicv3_icv_ap_write(ri->crm & 1, regno,
> gicv3_redist_affid(cs), value);
> 
> -    if (cs->gic->nmi_support) {
> +    if (cs->nmi_support) {
>          cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
>      } else {
>          cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
> @@ -1510,7 +1510,7 @@ static int icv_drop_prio(GICv3CPUState *cs, bool *nmi)
>              continue;
>          }
> 
> -        if (i == 0 && cs->gic->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
> +        if (i == 0 && cs->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
>              *papr1 &= (~ICV_AP1R_EL1_NMI);
>              *nmi = true;
>              return 0xff;
> @@ -2699,7 +2699,7 @@ static void ich_ap_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> 
>      trace_gicv3_ich_ap_write(ri->crm & 1, regno,
> gicv3_redist_affid(cs), value);
> 
> -    if (cs->gic->nmi_support) {
> +    if (cs->nmi_support) {
>          cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
>      } else {
>          cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
> @@ -2821,7 +2821,7 @@ static void ich_lr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
>      }
> 
>      /* Enforce RES0 bit in NMI field when FEAT_GICv3_NMI is not implemented */
> -    if (!cs->gic->nmi_support) {
> +    if (!cs->nmi_support) {
>          value &= ~ICH_LR_EL2_NMI;
>      }
> 
> The comments and commit message for patch 24 also need tweaking,
> because they are written assuming that FEAT_GICv3_NMI means
> "NMI support in the GIC proper", not "NMI support in the cpuif".

Yes, they're different, "FEAT_GICv3_NMI" is something beside PE, that is
the cpuif.

> 
> Since those changes are not too complicated, and I made them
> locally anyway since I wanted to confirm that my plan was
> workable, my proposal is that I will apply these fixes while
> I take this series into target-arm.next for 9.1.
> 
> So I've applied this series to target-arm.next with the above
> changes (preparatory to doing a pull request tail end of next
> week once we release 9.0). Let me know if you'd prefer something
> else.
> 

Thank you! I think that is good.

> thanks
> -- PMM