diff mbox series

[RFC,02/14] RISC-V: Add SBI STA extension definitions

Message ID 20230417103402.798596-3-ajones@ventanamicro.com (mailing list archive)
State Awaiting Upstream, archived
Headers show
Series RISC-V: Add steal-time support | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes or riscv/for-next

Commit Message

Andrew Jones April 17, 2023, 10:33 a.m. UTC
The SBI STA extension enables steal-time accounting. Add the
definitions it specifies.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Conor Dooley April 18, 2023, 6:43 p.m. UTC | #1
On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> The SBI STA extension enables steal-time accounting. Add the
> definitions it specifies.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 945b7be249c1..485b9ec20399 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -30,6 +30,7 @@ enum sbi_ext_id {
>  	SBI_EXT_HSM = 0x48534D,
>  	SBI_EXT_SRST = 0x53525354,
>  	SBI_EXT_PMU = 0x504D55,
> +	SBI_EXT_STA = 0x535441,

What is the sort order of this? Matching the spec ordering, or just
append-at-the-end?

Unrelated, but in checking that I saw that your SUSP stuff is in
master - you planning on resending that series?

Anyways, this does match the docs - but I'm quite hesitant to leave an
R-b when it's not merged yet.

Cheers,
Conor.

>  
>  	/* Experimentals extensions must lie within this range */
>  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
> @@ -236,6 +237,20 @@ enum sbi_pmu_ctr_type {
>  /* Flags defined for counter stop function */
>  #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
>  
> +/* SBI STA (steal-time accounting) extension */
> +enum sbi_ext_sta_fid {
> +	SBI_EXT_STA_SET_STEAL_TIME_SHMEM = 0,
> +};
> +
> +struct sbi_sta_struct {
> +	__le32 sequence;
> +	__le32 flags;
> +	__le64 steal;
> +	u8 preempted;
> +	u8 pad[47];
> +} __packed;
> +
> +/* SBI spec version fields */
>  #define SBI_SPEC_VERSION_DEFAULT	0x1
>  #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
>  #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Andrew Jones April 19, 2023, 8:15 a.m. UTC | #2
On Tue, Apr 18, 2023 at 07:43:51PM +0100, Conor Dooley wrote:
> On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> > The SBI STA extension enables steal-time accounting. Add the
> > definitions it specifies.
> > 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 945b7be249c1..485b9ec20399 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -30,6 +30,7 @@ enum sbi_ext_id {
> >  	SBI_EXT_HSM = 0x48534D,
> >  	SBI_EXT_SRST = 0x53525354,
> >  	SBI_EXT_PMU = 0x504D55,
> > +	SBI_EXT_STA = 0x535441,
> 
> What is the sort order of this? Matching the spec ordering, or just
> append-at-the-end?

I don't believe there's an established order. I've been going for spec
order, I think.

> 
> Unrelated, but in checking that I saw that your SUSP stuff is in
> master - you planning on resending that series?

I think I need to wait until it's been ratified.

> 
> Anyways, this does match the docs - but I'm quite hesitant to leave an
> R-b when it's not merged yet.

I could take your R-b now, and then if the spec changes, I'd drop it
when reposting the PoC after reworking it.

Thanks,
drew

> 
> Cheers,
> Conor.
> 
> >  
> >  	/* Experimentals extensions must lie within this range */
> >  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
> > @@ -236,6 +237,20 @@ enum sbi_pmu_ctr_type {
> >  /* Flags defined for counter stop function */
> >  #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
> >  
> > +/* SBI STA (steal-time accounting) extension */
> > +enum sbi_ext_sta_fid {
> > +	SBI_EXT_STA_SET_STEAL_TIME_SHMEM = 0,
> > +};
> > +
> > +struct sbi_sta_struct {
> > +	__le32 sequence;
> > +	__le32 flags;
> > +	__le64 steal;
> > +	u8 preempted;
> > +	u8 pad[47];
> > +} __packed;
> > +
> > +/* SBI spec version fields */
> >  #define SBI_SPEC_VERSION_DEFAULT	0x1
> >  #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
> >  #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
> > -- 
> > 2.39.2
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley April 19, 2023, 4:22 p.m. UTC | #3
On Wed, Apr 19, 2023 at 10:15:54AM +0200, Andrew Jones wrote:
> On Tue, Apr 18, 2023 at 07:43:51PM +0100, Conor Dooley wrote:
> > On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> > > The SBI STA extension enables steal-time accounting. Add the
> > > definitions it specifies.
> > > 
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > index 945b7be249c1..485b9ec20399 100644
> > > --- a/arch/riscv/include/asm/sbi.h
> > > +++ b/arch/riscv/include/asm/sbi.h
> > > @@ -30,6 +30,7 @@ enum sbi_ext_id {
> > >  	SBI_EXT_HSM = 0x48534D,
> > >  	SBI_EXT_SRST = 0x53525354,
> > >  	SBI_EXT_PMU = 0x504D55,
> > > +	SBI_EXT_STA = 0x535441,
> > 
> > What is the sort order of this? Matching the spec ordering, or just
> > append-at-the-end?
> 
> I don't believe there's an established order. I've been going for spec
> order, I think.
> 
> > 
> > Unrelated, but in checking that I saw that your SUSP stuff is in
> > master - you planning on resending that series?
> 
> I think I need to wait until it's been ratified.
> 
> > 
> > Anyways, this does match the docs - but I'm quite hesitant to leave an
> > R-b when it's not merged yet.
> 
> I could take your R-b now, and then if the spec changes, I'd drop it
> when reposting the PoC after reworking it.

Yah, sure.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.
Guo Ren Aug. 2, 2023, 11:32 p.m. UTC | #4
On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> The SBI STA extension enables steal-time accounting. Add the
> definitions it specifies.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 945b7be249c1..485b9ec20399 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -30,6 +30,7 @@ enum sbi_ext_id {
>  	SBI_EXT_HSM = 0x48534D,
>  	SBI_EXT_SRST = 0x53525354,
>  	SBI_EXT_PMU = 0x504D55,
> +	SBI_EXT_STA = 0x535441,
>  
>  	/* Experimentals extensions must lie within this range */
>  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
> @@ -236,6 +237,20 @@ enum sbi_pmu_ctr_type {
>  /* Flags defined for counter stop function */
>  #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
>  
> +/* SBI STA (steal-time accounting) extension */
> +enum sbi_ext_sta_fid {
> +	SBI_EXT_STA_SET_STEAL_TIME_SHMEM = 0,
> +};
> +
> +struct sbi_sta_struct {
> +	__le32 sequence;
> +	__le32 flags;
> +	__le64 steal;
Could we wrap the "sequence & steal" into one 64-bit variable? Then only
rv32 needs double READs, and only one ld instruction for rv64 ISA.

> +	u8 preempted;
> +	u8 pad[47];
> +} __packed;
> +
> +/* SBI spec version fields */
>  #define SBI_SPEC_VERSION_DEFAULT	0x1
>  #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
>  #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Guo Ren Aug. 3, 2023, 1:27 a.m. UTC | #5
On Wed, Apr 19, 2023 at 10:15:54AM +0200, Andrew Jones wrote:
> On Tue, Apr 18, 2023 at 07:43:51PM +0100, Conor Dooley wrote:
> > On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> > > The SBI STA extension enables steal-time accounting. Add the
> > > definitions it specifies.
> > > 
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > index 945b7be249c1..485b9ec20399 100644
> > > --- a/arch/riscv/include/asm/sbi.h
> > > +++ b/arch/riscv/include/asm/sbi.h
> > > @@ -30,6 +30,7 @@ enum sbi_ext_id {
> > >  	SBI_EXT_HSM = 0x48534D,
> > >  	SBI_EXT_SRST = 0x53525354,
> > >  	SBI_EXT_PMU = 0x504D55,
> > > +	SBI_EXT_STA = 0x535441,
> > 
> > What is the sort order of this? Matching the spec ordering, or just
> > append-at-the-end?
> 
> I don't believe there's an established order. I've been going for spec
> order, I think.
> 
> > 
> > Unrelated, but in checking that I saw that your SUSP stuff is in
> > master - you planning on resending that series?
> 
> I think I need to wait until it's been ratified.
Did you discuss the SBI_EXT_STA number of the SBI spec? Could you share
the link to me? Thx.

> 
> > 
> > Anyways, this does match the docs - but I'm quite hesitant to leave an
> > R-b when it's not merged yet.
> 
> I could take your R-b now, and then if the spec changes, I'd drop it
> when reposting the PoC after reworking it.
> 
> Thanks,
> drew
> 
> > 
> > Cheers,
> > Conor.
> > 
> > >  
> > >  	/* Experimentals extensions must lie within this range */
> > >  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
> > > @@ -236,6 +237,20 @@ enum sbi_pmu_ctr_type {
> > >  /* Flags defined for counter stop function */
> > >  #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
> > >  
> > > +/* SBI STA (steal-time accounting) extension */
> > > +enum sbi_ext_sta_fid {
> > > +	SBI_EXT_STA_SET_STEAL_TIME_SHMEM = 0,
> > > +};
> > > +
> > > +struct sbi_sta_struct {
> > > +	__le32 sequence;
> > > +	__le32 flags;
> > > +	__le64 steal;
> > > +	u8 preempted;
> > > +	u8 pad[47];
> > > +} __packed;
> > > +
> > > +/* SBI spec version fields */
> > >  #define SBI_SPEC_VERSION_DEFAULT	0x1
> > >  #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
> > >  #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
> > > -- 
> > > 2.39.2
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Andrew Jones Aug. 3, 2023, 6:48 a.m. UTC | #6
On Wed, Aug 02, 2023 at 09:27:51PM -0400, Guo Ren wrote:
> On Wed, Apr 19, 2023 at 10:15:54AM +0200, Andrew Jones wrote:
> > On Tue, Apr 18, 2023 at 07:43:51PM +0100, Conor Dooley wrote:
> > > On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> > > > The SBI STA extension enables steal-time accounting. Add the
> > > > definitions it specifies.
> > > > 
> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > ---
> > > >  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > > index 945b7be249c1..485b9ec20399 100644
> > > > --- a/arch/riscv/include/asm/sbi.h
> > > > +++ b/arch/riscv/include/asm/sbi.h
> > > > @@ -30,6 +30,7 @@ enum sbi_ext_id {
> > > >  	SBI_EXT_HSM = 0x48534D,
> > > >  	SBI_EXT_SRST = 0x53525354,
> > > >  	SBI_EXT_PMU = 0x504D55,
> > > > +	SBI_EXT_STA = 0x535441,
> > > 
> > > What is the sort order of this? Matching the spec ordering, or just
> > > append-at-the-end?
> > 
> > I don't believe there's an established order. I've been going for spec
> > order, I think.
> > 
> > > 
> > > Unrelated, but in checking that I saw that your SUSP stuff is in
> > > master - you planning on resending that series?
> > 
> > I think I need to wait until it's been ratified.
> Did you discuss the SBI_EXT_STA number of the SBI spec? Could you share
> the link to me? Thx.
>

The number is just "STA" in hex, as all SBI extensions are numbered. All
discussion of the spec was on the tech-prs mailing list[1]. Searching for
"steal" in the archives appears to pull up relevant threads.

[1] https://lists.riscv.org/g/tech-prs/messages

Thanks,
drew
Andrew Jones Aug. 3, 2023, 7:20 a.m. UTC | #7
On Wed, Aug 02, 2023 at 07:32:48PM -0400, Guo Ren wrote:
> On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> > The SBI STA extension enables steal-time accounting. Add the
> > definitions it specifies.
> > 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 945b7be249c1..485b9ec20399 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -30,6 +30,7 @@ enum sbi_ext_id {
> >  	SBI_EXT_HSM = 0x48534D,
> >  	SBI_EXT_SRST = 0x53525354,
> >  	SBI_EXT_PMU = 0x504D55,
> > +	SBI_EXT_STA = 0x535441,
> >  
> >  	/* Experimentals extensions must lie within this range */
> >  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
> > @@ -236,6 +237,20 @@ enum sbi_pmu_ctr_type {
> >  /* Flags defined for counter stop function */
> >  #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
> >  
> > +/* SBI STA (steal-time accounting) extension */
> > +enum sbi_ext_sta_fid {
> > +	SBI_EXT_STA_SET_STEAL_TIME_SHMEM = 0,
> > +};
> > +
> > +struct sbi_sta_struct {
> > +	__le32 sequence;
> > +	__le32 flags;
> > +	__le64 steal;
> Could we wrap the "sequence & steal" into one 64-bit variable? Then only
> rv32 needs double READs, and only one ld instruction for rv64 ISA.

That's possible, but we'd have to reduce the size of steal by whatever
size we decide is sufficient for sequence. In order to do that we'll
need to discuss the size reduction proposals and their justifications
at the spec level. If you'd like to make that proposal, then please
create an issue at [1]. But, I don't think it should be necessary.
There's non-normative text in the spec that says "This sequence field
enables the value of the steal field to be read by supervisor-mode
software executing in a 32-bit environment.", which implies to me
that we could optimize the read in a 64-bit environment by neglecting
to read sequence at all.

[1] https://github.com/riscv-non-isa/riscv-sbi-doc

Thanks,
drew

> 
> > +	u8 preempted;
> > +	u8 pad[47];
> > +} __packed;
> > +
> > +/* SBI spec version fields */
> >  #define SBI_SPEC_VERSION_DEFAULT	0x1
> >  #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
> >  #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
> > -- 
> > 2.39.2
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
Guo Ren Aug. 5, 2023, 1:34 a.m. UTC | #8
On Thu, Aug 3, 2023 at 2:48 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Aug 02, 2023 at 09:27:51PM -0400, Guo Ren wrote:
> > On Wed, Apr 19, 2023 at 10:15:54AM +0200, Andrew Jones wrote:
> > > On Tue, Apr 18, 2023 at 07:43:51PM +0100, Conor Dooley wrote:
> > > > On Mon, Apr 17, 2023 at 12:33:50PM +0200, Andrew Jones wrote:
> > > > > The SBI STA extension enables steal-time accounting. Add the
> > > > > definitions it specifies.
> > > > >
> > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/sbi.h | 15 +++++++++++++++
> > > > >  1 file changed, 15 insertions(+)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > > > index 945b7be249c1..485b9ec20399 100644
> > > > > --- a/arch/riscv/include/asm/sbi.h
> > > > > +++ b/arch/riscv/include/asm/sbi.h
> > > > > @@ -30,6 +30,7 @@ enum sbi_ext_id {
> > > > >         SBI_EXT_HSM = 0x48534D,
> > > > >         SBI_EXT_SRST = 0x53525354,
> > > > >         SBI_EXT_PMU = 0x504D55,
> > > > > +       SBI_EXT_STA = 0x535441,
> > > >
> > > > What is the sort order of this? Matching the spec ordering, or just
> > > > append-at-the-end?
> > >
> > > I don't believe there's an established order. I've been going for spec
> > > order, I think.
> > >
> > > >
> > > > Unrelated, but in checking that I saw that your SUSP stuff is in
> > > > master - you planning on resending that series?
> > >
> > > I think I need to wait until it's been ratified.
> > Did you discuss the SBI_EXT_STA number of the SBI spec? Could you share
> > the link to me? Thx.
> >
>
> The number is just "STA" in hex, as all SBI extensions are numbered. All
> discussion of the spec was on the tech-prs mailing list[1]. Searching for
> "steal" in the archives appears to pull up relevant threads.
>
> [1] https://lists.riscv.org/g/tech-prs/messages
Thx for the information; It could guide me to send the SBI_EXT_PVLOCK request.

>
> Thanks,
> drew
>
>
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 945b7be249c1..485b9ec20399 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -30,6 +30,7 @@  enum sbi_ext_id {
 	SBI_EXT_HSM = 0x48534D,
 	SBI_EXT_SRST = 0x53525354,
 	SBI_EXT_PMU = 0x504D55,
+	SBI_EXT_STA = 0x535441,
 
 	/* Experimentals extensions must lie within this range */
 	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
@@ -236,6 +237,20 @@  enum sbi_pmu_ctr_type {
 /* Flags defined for counter stop function */
 #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
 
+/* SBI STA (steal-time accounting) extension */
+enum sbi_ext_sta_fid {
+	SBI_EXT_STA_SET_STEAL_TIME_SHMEM = 0,
+};
+
+struct sbi_sta_struct {
+	__le32 sequence;
+	__le32 flags;
+	__le64 steal;
+	u8 preempted;
+	u8 pad[47];
+} __packed;
+
+/* SBI spec version fields */
 #define SBI_SPEC_VERSION_DEFAULT	0x1
 #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
 #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f