Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip #617

Shehrozkashif · 2025-04-16T10:10:39Z

This pr includes csr vsip,hegeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip

ThinkOpenly

I did not finish the review, but thought this feedback would be helpful for now.

ThinkOpenly · 2025-04-16T14:23:43Z

arch/csr/H/hgeie.yaml

+  The `hgeie` register is an HSXLEN-bit read/write register that contains enable bits
+  for the guest external interrupts at this hart. Each bit in `hgeie` corresponds to
+  a guest external interrupt and determines whether that interrupt is enabled.


Hew to the text in the spec:

The hgeie register is an HSXLEN-bit read/write register that contains enable bits for the guest external interrupts at this hart. Guest external interrupt number i corresponds with bit i in hgeie.

Guest external interrupts represent interrupts directed to individual virtual machines at VS-level. If a RISC-V platform supports placing a physical device under the direct control of a guest OS with minimal hypervisor intervention (known as pass-through or direct assignment between a virtual machine and the physical device), then, in such circumstance, interrupts from the device are intended for a specific virtual machine.

[Note]
Support for guest external interrupts requires an interrupt controller that can collect virtual-machine-directed interrupts separately from other interrupts.

The number of bits implemented in hgeie for guest external interrupts is UNSPECIFIED and may be zero. This number is known as GEILEN. The least-significant bits are implemented first, apart from bit 0. Hence, if GEILEN is nonzero, bits GEILEN:1 shall be writable in hgeie, and all other bit positions shall be read-only zeros.

Register hgeie selects the subset of guest external interrupts that cause a supervisor-level (HS-level) guest external interrupt. The enable bits in hgeie do not affect the VS-level external interrupt signal selected from hgeip by hstatus.VGEIN.

(I culled content that was specific to hgeip or seems to be specific to the extension, not hgeie itself.)

Got it, I’ll fix it to be more like the spec.

Restore backticks around all the CSRs in all the description fields.

If you could make these LONG lines shorter (less than about 70-80 characters), that is preferred.

arch/csr/H/hgeie.yaml

arch/csr/H/hgeip.yaml

arch/csr/H/hideleg.yaml

ThinkOpenly · 2025-04-23T19:20:57Z

arch/csr/H/hie.yaml

+length: SXLEN
+fields:
+  SGEIE:
+    location: 0


ThinkOpenly · 2025-04-23T19:21:59Z

arch/csr/H/hie.yaml

+      to VS-mode based on the `hgeie` setting.
+
+  VSEIE:
+    location: 4


ThinkOpenly · 2025-04-23T19:22:23Z

arch/csr/H/hie.yaml

+      to be processed based on the configuration in `hvip` and other platform-specific sources.
+
+  VSTIE:
+    location: 8


ThinkOpenly · 2025-04-23T19:22:33Z

arch/csr/H/hie.yaml

+      VS-level timer interrupt enable bit. When set, allows VS-level timer interrupts to be processed
+      based on the `hvip` configuration and any platform-specific timer interrupts.
+
+  VSSIE:


ThinkOpenly · 2025-04-23T19:30:15Z

arch/csr/H/hvip.yaml

+      Pending interrupt bit for VS-level external interrupts. This bit is writable and
+      is set to 1 to assert a VS-level external interrupt.
+
+  VSTIP:


ThinkOpenly · 2025-04-23T19:30:25Z

arch/csr/H/hvip.yaml

+      is set to 1 to assert a VS-level timer interrupt.
+
+  VSSIP:
+    location: 5


ThinkOpenly · 2025-04-23T19:31:24Z

arch/csr/H/hvip.yaml

+      Pending interrupt bit for VS-level software interrupts. This bit is writable and
+      is set to 1 to assert a VS-level software interrupt.
+
+  reserved_6_15:


just "reserved"
(I'm not sure this is the preferred way of noting always-zero bits, but we'll roll with it for now...)

We don't need to list reserved bits at all. If there is no field at a bit, it is assumed to be reserved

ThinkOpenly · 2025-04-23T19:31:44Z

arch/csr/H/hvip.yaml

+      is set to 1 to assert a VS-level software interrupt.
+
+  reserved_6_15:
+    location: 6


"15-11,9-7,5-3,1-0"

ThinkOpenly · 2025-04-23T19:32:51Z

arch/csr/H/vsie.yaml

+  The standard portion (bits 15:0) of vsie is formatted as follows:
+  15 14 13 12 10 9 8 6 5 4 2 1 0
+  0 LCOFIE 0 SEIE 0 STIE 0 SSIE 0
+  2 1 3 1 3 1 3 1 1
+


This is obviously not formatted well. Delete it. The fields information below covers the same information.

…e, vsscratch, hgeip

dhower-qc

This is a complicated set of CSRs. I've added a bunch of comments, but note that we're likely to need multiple review passes before it's ready.

dhower-qc · 2025-04-29T13:25:43Z

arch/csr/H/hgeie.yaml

+description: |
+  The hgeie register is an HSXLEN-bit read/write register that contains enable bits for the guest external interrupts at this hart. Guest external interrupt number i corresponds with bit i in hgeie.
+
+  Guest external interrupts represent interrupts directed to individual virtual machines at VS-level. If a RISC-V platform supports placing a physical device under the direct control of a guest OS with minimal hypervisor intervention (known as pass-through or direct assignment between a virtual machine and the physical device), then, in such circumstance, interrupts from the device are intended for a specific virtual machine.
+
+  [Note]
+  Support for guest external interrupts requires an interrupt controller that can collect virtual-machine-directed interrupts separately from other interrupts.
+
+  The number of bits implemented in hgeie for guest external interrupts is UNSPECIFIED and may be zero. This number is known as GEILEN. The least-significant bits are implemented first, apart from bit 0. Hence, if GEILEN is nonzero, bits GEILEN:1 shall be writable in hgeie, and all other bit positions shall be read-only zeros.
+
+  Register hgeie selects the subset of guest external interrupts that cause a supervisor-level (HS-level) guest external interrupt. The enable bits in hgeie do not affect the VS-level external interrupt signal selected from hgeip by hstatus.VGEIN.


Let's change this to the new structured description. Should look like:

description: - id: csr-hgeie-purpose normative: true text: | The hgeie register is an HSXLEN-bit read/write register that contains enable bits for the guest external interrupts at this hart. - id: csr-hgeie-bits normative: true text: Guest external interrupt number i corresponds with bit i in hgeie. - id: csr-hgeie-interrupts normative: true text: | Guest external interrupts represent interrupts directed to individual virtual machines at VS-level. If a RISC-V platform supports placing a physical device under the direct control of a guest OS with minimal hypervisor intervention (known as pass-through or direct assignment between a virtual machine and the physical device), then, in such circumstance, interrupts from the device are intended for a specific virtual machine. - id: csr-hgeie-controller-req normative: false text: | Support for guest external interrupts requires an interrupt controller that can collect virtual-machine-directed interrupts separately from other interrupts. - id: csr-hgeie-geilen normative: true text: | The number of bits implemented in hgeie for guest external interrupts is UNSPECIFIED and may be zero. This number is known as `GEILEN`. - id: csr-hgeie-bitorder normative: true text: | The least-significant bits are implemented first, apart from bit 0. - id: csr-hgeie-nonzero-geilen normative: true text: | Hence, if GEILEN is nonzero, bits `GEILEN`:1 shall be writable in hgeie, and all other bit positions shall be read-only zeros. - id: csr-hgeie-select normative: true text: | Register hgeie selects the subset of guest external interrupts that cause a supervisor-level (HS-level) guest external interrupt. - id: csr-hgeie-hgeip normative: true text: | The enable bits in hgeie do not affect the VS-level external interrupt signal selected from hgeip by hstatus.VGEIN.

We should do this for all the descriptiong in the PR.

dhower-qc · 2025-04-29T13:29:20Z

arch/csr/H/hgeie.yaml

+    location_rv32: 31-1
+    location_rv64: 63-1
+    type: RW
+    reset_value: 0


UNDEFINED_LEGAL

dhower-qc · 2025-04-29T13:32:16Z

arch/csr/H/hgeie.yaml

+definedBy: H
+length: SXLEN
+fields:
+  GEI_ENABLE:


add:

long_name: Guest external interrupt enable bits

dhower-qc · 2025-04-29T13:33:03Z

arch/csr/H/hgeie.yaml

+    description: |
+      The number of bits implemented in hgeie for guest external interrupts is UNSPECIFIED and may be zero. This number is known as GEILEN. The least-significant bits are implemented first, apart from bit 0. Hence, if GEILEN is nonzero, bits GEILEN:1 shall be writable in hgeie, and all other bit positions shall be read-only zeros in  hgeie.
+      Register hgeie selects the subset of guest external interrupts that cause a supervisor-level (HS-level) guest external interrupt. The enable bits in hgeie do not affect the VS-level external interrupt signal selected from hgeip by hstatus.VGEIN.


The description here is already stated in the description of the CSR overall. For this field, I would just say "Enable bits; see description of hgeie".

dhower-qc · 2025-04-29T13:35:02Z

arch/csr/H/hgeie.yaml

+  GEI_ENABLE:
+    location_rv32: 31-1
+    location_rv64: 63-1
+    type: RW


Since this field is WARL, the type is configuration-dependent; it's read-only if GEILEN is zero, and read/write otherwise. As a complication, UDB has encoded GEILEN as NUM_EXTERNAL_GUEST_INTERRUPTS.

type(): return NUM_EXTERNAL_GUEST_INTERRUPTS > 0 ? CsrFieldType::RW : CsrFieldType::RO;

dhower-qc · 2025-04-29T14:18:14Z

arch/csr/H/hip.yaml

+
+  VSTIP:
+    location: 6
+    type: RO


Should be RO-H

dhower-qc · 2025-04-29T14:51:51Z

arch/csr/H/hip.yaml

+
+  VSSIP:
+    location: 2
+    type: RO


dhower-qc · 2025-04-29T14:52:20Z

arch/csr/H/hip.yaml

+      This bit is the logical OR of `vstip` from `hvip` and any other timer interrupt directed to VS-level.
+
+  VSSIP:
+    location: 2


alias: hvip.VSSIP sw_write(csr_value): CSR[hvip].VSSIP = csr_value.VSSIP;

dhower-qc · 2025-04-29T14:57:21Z

arch/csr/H/hip.yaml

+  VSTIP:
+    location: 6
+    type: RO
+    reset_value: 0
+    description: |
+      Pending interrupt bit for VS-level timer interrupts (VSTI).
+      This bit is the logical OR of `vstip` from `hvip` and any other timer interrupt directed to VS-level.


[[[ NOTE: I wrote this before I realized we haven't added any Sstc CSRs yet. Can you also add them so that this doesn't fall through the cracks? ]]]

VSTIP is altered by the Sstc extension.

19.2.2. Hypervisor Interrupt (hvip, hip, and hie) Registers
This extension modifies the description of the VSTIP/VSTIE bits in the hip/hie registers as follows:

Bits hip.VSTIP and hie.VSTIE are the interrupt-pending and interrupt-enable bits for VS-level timer interrupts. VSTIP is read-only in hip, and is the logical-OR of hvip.VSTIP and the timer interrupt signal resulting from vstimecmp (if vstimecmp is implemented). The hip.VSTIP bit, in response to timer interrupts generated by vstimecmp, is set and cleared by writing vstimecmp with a value that respectively is less than or equal to, or greater than, the current (time + htimedelta) value. The hip.VSTIP bit remains defined while V=0 as well as V=1.

This will require the function form of reset_value() on the field.

It will also require a sw_read() function for the CSR overall. Something like:

sw_read(): | Bits<7> vstip_bit = 0; if (implemented?(ExtensionName::Sstc)) { if ((CSR[hvip].VSTIP == 1) | (CSR[vstimecmp].VALUE > (read_mtime() + CSR[htimedelta].DELTA)) { vstip_bit = 7'b1000000; } } Bits<3> vssip_bit = CSR[hvip].VSSIP == 0 ? 0 : 3'b100; # ... return lcofi_bit | sgeip_bit | vseip_bit | vstip_bit | vssip_bit;

sure ill add Sstc CSRs I believe these are they ?

"Sstc" Extension for Supervisor-mode Timer Interrupts, Version 1.0.0 123
16.1. Machine and Supervisor Level Additions 123
16.1.1. Supervisor Timer Register (stimecmp) 123
16.1.2. Machine Interrupt Registers (mip and mie) 124
16.1.3. Supervisor Interrupt Registers (sip and sie) 124
16.1.4. Machine Counter-Enable Register (mcounteren) 124
16.2. Hypervisor Extension Additions 124
16.2.1. Virtual Supervisor Timer Register (vstimecmp) 124
16.2.2. Hypervisor Interrupt Registers (hvip, hip, and hie) 125
16.2.3. Hypervisor Counter-Enable Register (hcounteren) 125
16.3. Environment Config (menvcfg/henvcfg) Support

dhower-qc · 2025-04-29T14:58:08Z

arch/csr/H/hip.yaml

@@ -0,0 +1,46 @@
+# yaml-language-server: $schema=../../../schemas/csr_schema.json


This CSR needs a sw_read function that reflects the following (I've started a skeleton of it in the VSSIP comment):

SGEIP is read-only in hip, and is 1 if and only if the bitwise logical-AND of CSRs hgeip and hgeie is nonzero in any bit.

VSEIP is read-only in hip, and is the logical-OR of these interrupt sources:

bit VSEIP of hvip;

the bit of hgeip selected by hstatus.VGEIN; and

any other platform-specific external interrupt signal directed to VS-level.

VSTIP is read-only in hip, and is the logical-OR of hvip.VSTIP and any other platform-specific timer interrupt signal directed to VS-level.

VSSIP in hip is an alias (writable) of the same bit in hvip

Shehrozkashif requested a review from dhower-qc as a code owner April 16, 2025 10:10

ThinkOpenly changed the title ~~Add H csr vsip,hegeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip~~ Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip Apr 16, 2025

ThinkOpenly requested changes Apr 16, 2025

View reviewed changes

Shehrozkashif force-pushed the H_CSRs branch from d159605 to facc88f Compare April 18, 2025 20:10

Shehrozkashif requested a review from ThinkOpenly April 22, 2025 11:17

ThinkOpenly requested changes Apr 23, 2025

View reviewed changes

Shehrozkashif force-pushed the H_CSRs branch from facc88f to ef52648 Compare April 28, 2025 19:48

feat(csr): add H-mode CSRs vsip, hegeie, hideleg, hie, hip, hvip, vsi…

0e1a7fb

…e, vsscratch, hgeip

Shehrozkashif force-pushed the H_CSRs branch from ef52648 to 0e1a7fb Compare April 29, 2025 05:08

dhower-qc requested changes Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip #617

Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip #617

Shehrozkashif commented Apr 16, 2025 •

edited

Loading

ThinkOpenly left a comment

ThinkOpenly Apr 16, 2025

Shehrozkashif Apr 17, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

dhower-qc Apr 28, 2025

ThinkOpenly Apr 23, 2025

ThinkOpenly Apr 23, 2025

dhower-qc left a comment

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

dhower-qc Apr 29, 2025

Shehrozkashif May 5, 2025

dhower-qc Apr 29, 2025

		@@ -0,0 +1,46 @@
		# yaml-language-server: $schema=../../../schemas/csr_schema.json

Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip #617

Are you sure you want to change the base?

Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip #617

Conversation

Shehrozkashif commented Apr 16, 2025 • edited Loading

ThinkOpenly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhower-qc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shehrozkashif commented Apr 16, 2025 •

edited

Loading