Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906
Christoph linked here on his submission to Linux of a fix for this, so I am reviving this to leave a couple comments :) On Thu, Feb 15, 2024 at 02:24:02PM +1000, Alistair Francis wrote: > On Mon, Feb 5, 2024 at 6:37 PM Christoph Müllner > wrote: > > On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis > > wrote: > > > On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei > > > wrote: > > > > ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT; > > > > > > Unfortunately we won't be able to take this upstream. This is core > > > QEMU RISC-V code that is now being changed against the spec. I think > > > adding the CSR is fine, but we can't take this core change. > > > > > > A fix that works for everyone should be supporting the th_mxstatus > > > CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way > > > guests can detect that the bit isn't set and not use the reserved bits > > > in the PTE. From my understanding the extra PTE bits are related to > > > cache control in the hardware, which we don't need here > > > > Sounds good! Let me recap the overall plan: > > * QEMU does not emulate MAEE, but signals that MAEE is not available > > by setting TH_MXSTATUS_MAEE to 0. > > Yep! > > > * Consequence: The c906 emulation does not enable any page-base memory > > attribute mechanism. > > Exactly > > > * OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides > > that information to user-space (e.g. DTB). > > To the kernel, but yep! > > > * The current Linux errata code will be enhanced to not assume MAEE > > for each core with T-Head vendor ID, but also query the MAEE bit and > > ensure it is set. > > I feel like it should already do that :) It doesn't quite do this right now. It only makes the assumption for CPUs where marchid and mvendorid are zero. The c908, and I think Guo Ren confirmed it will be the case going forward, sets these to non-zero values. We should have always required a dt property be set, rather than using m*id, but we can't go back on that for these devices. Going forward, if there are more CPUs that want to use this e.g. C908 in MAEE mode (it can do svpbmt too) I'm gonna require it is explicitly set in DT. > > * Booting on a QEMU c906 will not enable MAEE, but will still boot. > > That's the hope > > > > > We never had a complete MAEE implementation upstream, because that's not > > needed. > > But until recently we could mess with reserved bits how we want. > > Now that QEMU is more restrictive about reserved bits in the PTEs, we > > need to address this in the kernel. > > > > The downside is, that the kernel now sees a c906 CPU without MAEE and > > such a CPU does not exist. > > Yeah, that is the downside. But in reality a CPU could exist that > doesn't allow seeing MAEE, so I don't think it's that insane. Aye, and the case of a new CPU disabling it but not setting m*id will be a pain. > > But that's fine, because this does not require extra code to handle this > > case. > > Also, the additional check for the MAEE bit in the kernel errata > > probing code is likely needed anyway for future T-Head CPUs. > > Exactly I don't really want to have extension detection side channels like this in Linux if it can be at all avoided, I'd rather get this information from DT or ACPI. The marchid == mvendorid == 0x0 case has a pretty high chance of needing some special handling in the future though, so something targeting those cases when there's some demonstrable problem seems fair to me. Cheers, Conor. signature.asc Description: PGP signature
Re: [RFC 1/2] hw/riscv: Add server platform reference machine
On Mon, Mar 04, 2024 at 06:25:39PM +0800, Fei Wu wrote: > +name = riscv_isa_string(cpu_ptr); > +qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name); > +g_free(name); Please use riscv_isa_write_fdt() here so that you populate more than the deprecated property here please. Thanks, Conor. signature.asc Description: PGP signature
Re: [PATCH v3 3/6] target/riscv: add remaining named features
> > No they can't. For a "regular extension" you populate the DT with the > > extension. For these extensions it has to put negated properties in the > > DT, otherwise it is incorrectly describing the hardware it is emulating. > > That is handling them differently in my book! If QEMU generates an > > incorrect DT representation of the hardware it is emulating, that's a > > QEMU bug. > > QEMU listing the extensions that it supports seems to me to be the > correct approach. > > It's clunky that the list of "extensions" are constantly changing. > There isn't much we can do about that from a QEMU perspective though. > > Listing the hardware and what it supports is the job of the DT. > > I see your concern about what happens if the "extensions" are disabled > though. Realislity they probably never will be. Yeah, it's something we can sweep under the rug unless/until someone wants to disable these things. > > > Linux or whatever software consuming > > > the hardware descriptions may want to distrust the absence of newly > > > named feature extensions and do their own checks, but that's not QEMU's > > > concern. > > > > Software should be able to trust that the DT describes the system > > correctly. I can only speak for Linux here, but validating the DT is not > > the job of the running kernel - it should be a correct description. > > AFAIK the DT is correct. We are describing the hardware within the > scope of the DT spec. > > If a new node exists that describes what the hardware does not support > we can update to support that as well. It won't be a new node property, it'll just be negated properties - eg riscv,isa-extensions = ..., "no-zicclsm"; That's what I mean when I say that these will not be able to be treated in the same way as any other extension, but it only applies iff someone wants to disable them. This isn't just a QEMU problem, but QEMU is the bleeding edge of "hardware" support, so it's cropping up here first (or maybe only :)) > > > Actually, being able to disable these newly named features allows > > > Linux and other software to test how they behave when the feature goes > > > away. > > > > That's helpful sure, but it doesn't absolve QEMU of having to correctly > > generate a DT. > > I'm pretty sure there isn't anything for us to do differently here > right? It's just a bad situation that we are trying to support. Until someone wants to turn them off, you can avoid doing anything differently, just like this amazing ascii art I found: _,-\/-,_ \ / \_.._/ _,/\,_ / \ / \ ,\ )( /, (__/ .''. \__) \,_|| / | ||\ | | /|| \| () || () // || || // || || // || || //|| /\ -- '' -'-' ^^')( '^^-- '' -'-' miK (==) `~` Hopefully posting ostriches on the QEMU list isn't grounds for a ban, Conor. signature.asc Description: PGP signature
Re: [PATCH v3 3/6] target/riscv: add remaining named features
On Thu, Feb 15, 2024 at 08:11:45PM +0100, Andrew Jones wrote: > On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote: > > On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote: > > > On Thu, Feb 15, 2024 at 01:33:47PM +, Conor Dooley wrote: > > > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza wrote: > > > > > The RVA22U64 and RVA22S64 profiles mandates certain extensions that, > > > > > until now, we were implying that they were available. > > > > > > > > > > We can't do this anymore since named features also has a riscv,isa > > > > > entry. Let's add them to riscv_cpu_named_features[]. > > > > > > > > > > Instead of adding one bool for each named feature that we'll always > > > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool > > > > > in > > > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > > > > named features will point to it. This also means that KVM won't see > > > > > these features as always enable, which is our intention. > > > > > > > > > > If any accelerator adds support to disable one of these features, > > > > > we'll > > > > > have to promote them to regular extensions and allow users to disable > > > > > it > > > > > via command line. > > > > > > > > > > After this patch, here's the riscv,isa from a buildroot using the > > > > > 'rva22s64' CPU: > > > > > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > > > > present in "u" profiles? > > > > > > "s" profiles mandate all the "u" profile mandatory extensions. For example > > > 6.2.2 says > > > > > > """ > > > The RVA22S64 mandatory unprivileged extensions include all the mandatory > > > unprivileged > > > extensions in RVA22U64. > > > """ > > > > Doesn't that rule out emulating misaligned access in s-mode if you want > > to be profile compliant? > > That's how I interpret it, but I'll defer to a profile spec author, or > at least to somebody more confident of their spec interpretation skills. Hmm, actually it doesn't. Your firmware just needs to _also_ implement it. So your OS kernel could test whether or not the misaligned access performance is beans and then choose to emulate misaligned access itself. Not ideal, but better than I thought. > > > > > # cat /proc/device-tree/cpus/cpu@0/riscv,isa > > > > > rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_ > > > > > zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_ > > > > > zbs_zkt_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > > > > > > > I want to raise my frustration with the crock we've been given here by > > > > RVI. Any "named feature" that just creates a name for something that > > > > already is assumed is completely useless, and DT property that is used > > > > to communicate it's presence cannot be used - instead the property needs > > > > to be inverted - indicating the absence of that named feature. > > > > > > > > Without the inversion, software that parses "riscv,isa" cannot make any > > > > determination based on the absence of the property - it could be parsing > > > > an old DT that does not have the property or it could be parsing the DT > > > > of a system that does not support the extension. > > > > > > I'm guessing any platform which wants to advertise that it's compliant > > > with a profile will update its hardware descriptions to ensure all the > > > profile's mandatory extensions are presented. But, I think I understand > > > your concern. If somebody is parsing the ISA string as way to determine > > > if the platform is compliant with a profile, then they may get a false > > > negative due to the ISA string missing a newly named feature. > > > > Nah, you misunderstand me. I don't care at all about profiles or > > checking for compliance with one. I'm only interested in how my software > > can check that some feature is (or is not) supported. This creating a name > > for something implicit business is not a problem in and of itself, but > > putting then into "riscv,isa" is a pointless act
Re: [PATCH v3 3/6] target/riscv: add remaining named features
On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote: > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote: > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza wrote: > > > The RVA22U64 and RVA22S64 profiles mandates certain extensions that, > > > until now, we were implying that they were available. > > > > > > We can't do this anymore since named features also has a riscv,isa > > > entry. Let's add them to riscv_cpu_named_features[]. > > > > > > Instead of adding one bool for each named feature that we'll always > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > > named features will point to it. This also means that KVM won't see > > > these features as always enable, which is our intention. > > > > > > If any accelerator adds support to disable one of these features, we'll > > > have to promote them to regular extensions and allow users to disable it > > > via command line. > > > > > > After this patch, here's the riscv,isa from a buildroot using the > > > 'rva22s64' CPU: > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > > present in "u" profiles? > > "s" profiles mandate all the "u" profile mandatory extensions. For example > 6.2.2 says > > """ > The RVA22S64 mandatory unprivileged extensions include all the mandatory > unprivileged > extensions in RVA22U64. > """ Doesn't that rule out emulating misaligned access in s-mode if you want to be profile compliant? > > > # cat /proc/device-tree/cpus/cpu@0/riscv,isa > > > rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_ > > > zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_ > > > zbs_zkt_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > > > I want to raise my frustration with the crock we've been given here by > > RVI. Any "named feature" that just creates a name for something that > > already is assumed is completely useless, and DT property that is used > > to communicate it's presence cannot be used - instead the property needs > > to be inverted - indicating the absence of that named feature. > > > > Without the inversion, software that parses "riscv,isa" cannot make any > > determination based on the absence of the property - it could be parsing > > an old DT that does not have the property or it could be parsing the DT > > of a system that does not support the extension. > > I'm guessing any platform which wants to advertise that it's compliant > with a profile will update its hardware descriptions to ensure all the > profile's mandatory extensions are presented. But, I think I understand > your concern. If somebody is parsing the ISA string as way to determine > if the platform is compliant with a profile, then they may get a false > negative due to the ISA string missing a newly named feature. Nah, you misunderstand me. I don't care at all about profiles or checking for compliance with one. I'm only interested in how my software can check that some feature is (or is not) supported. This creating a name for something implicit business is not a problem in and of itself, but putting then into "riscv,isa" is a pointless activity as it communicates nothing. > I'm not > sure how much of a problem that will be in practice, though, since testing > for profile compliance, just for the sake of it, doesn't seem very useful. > Software really only needs to know which extensions are available and if > it's an old feature that got newly named, then software likely already > has another way of detecting it. Right. That part is fine, but creating extensions for these things we previously assumed present gives me the impression that creating systems that do not support these features is valid. IFF that does happen, removing the string from "riscv,isa" isn't going to be able to communicate that the feature is unsupported. The commit message here says: > > > If any accelerator adds support to disable one of these features, we'll > > > have to promote them to regular extensions and allow users to disable it > > > via command line. Which is part of what prompted me here, since they cannot be handled in the same way that "regular extensions" are. > > This is part of why I deprecated `riscv,isa`. It's the same problem as > > with "zifencei" et al - does a system with `riscv,isa = "rv64imac"` > > support fence.i? > > Yes, there's a handful of these messy things and the first profiles > expose them since they're trying to define them. Fingers crossed that > the next profiles won't have to name old features. FWIW, I at least > don't see any "This is a new extension name for this feature" notes in > the RVA23 profile. > > Thanks, > drew signature.asc Description: PGP signature
Re: [PATCH v3 3/6] target/riscv: add remaining named features
On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza wrote:
> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
> until now, we were implying that they were available.
>
> We can't do this anymore since named features also has a riscv,isa
> entry. Let's add them to riscv_cpu_named_features[].
>
> Instead of adding one bool for each named feature that we'll always
> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> named features will point to it. This also means that KVM won't see
> these features as always enable, which is our intention.
>
> If any accelerator adds support to disable one of these features, we'll
> have to promote them to regular extensions and allow users to disable it
> via command line.
>
> After this patch, here's the riscv,isa from a buildroot using the
> 'rva22s64' CPU:
Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
present in "u" profiles?
> # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
> zbs_zkt_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
I want to raise my frustration with the crock we've been given here by
RVI. Any "named feature" that just creates a name for something that
already is assumed is completely useless, and DT property that is used
to communicate it's presence cannot be used - instead the property needs
to be inverted - indicating the absence of that named feature.
Without the inversion, software that parses "riscv,isa" cannot make any
determination based on the absence of the property - it could be parsing
an old DT that does not have the property or it could be parsing the DT
of a system that does not support the extension.
This is part of why I deprecated `riscv,isa`. It's the same problem as
with "zifencei" et al - does a system with `riscv,isa = "rv64imac"`
support fence.i?
Cheers,
Conor.
>
> Signed-off-by: Daniel Henrique Barboza
> ---
> target/riscv/cpu.c | 42 +++---
> target/riscv/cpu_cfg.h | 6 ++
> target/riscv/tcg/tcg-cpu.c | 2 ++
> 3 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 28d3cfa8ce..94843c4f6e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
> ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
> ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> +ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
> ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
> @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
> ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
> +ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
> ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
> ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
> @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> +ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> +ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0,
> ext_always_enabled),
> ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> +ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
> ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
> ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
> @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig
> riscv_cpu_experimental_exts[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +#define ALWAYS_ENABLED_FEATURE(_name) \
> +{.name = _name, \
> + .offset = CPU_CFG_OFFSET(ext_always_enabled),
Re: [PATCH 2/2] target/riscv: Support xtheadmaee for thead-c906
On Tue, Jan 30, 2024 at 12:43:25PM +0100, Christoph Müllner wrote: > On Tue, Jan 30, 2024 at 12:12 PM LIU Zhiwei > wrote: > > > > thead-c906 uses some flags in pte [60-63] bits. It has history reasons that > > SVPBMT didn't exist when thead-c906 came to world. > > > > We named this feature as xtheadmaee. this feature is controlled by an custom > > CSR named mxstatus, whose maee field encodes whether enable the pte [60-63] > > bits. > > > > The sections "5.2.2.1 Page table structure" and "15.1.7.1 M-mode extension > > status register (MXSTATUS)" in document[1] give the detailed information > > about its design. > > I would prefer if we would not define an extension like XTheadMaee > without a specification. > The linked document defines the bit MAEE in a custom CSR, but the > scope of XTheadMaee > is not clearly defined (the term XTheadMaee is not even part of the PDF). > > We have all the XThead* extensions well described here: > https://github.com/T-head-Semi/thead-extension-spec/tree/master > And it would not be much effort to add XTheadMaee there as well. Yeah, I was gonna request exactly this, so glad to see you beat me to the punch. It would be really great if this was done, particularly if this xmaee is going to appear in devicetrees and sooner or later is gonna want to be documented in a binding. Cheers, Conor. > For those who don't know the context of this patch, here is the c906 > boot regression report from Björn: > https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04766.html signature.asc Description: PGP signature
Re: qemu riscv, thead c906, Linux boot regression
On Wed, Jan 24, 2024 at 02:27:10PM +0100, Björn Töpel wrote:
> Conor Dooley writes:
>
> > On Wed, Jan 24, 2024 at 01:49:51PM +0100, Björn Töpel wrote:
> >> Hi!
> >>
> >> I bumped the RISC-V Linux kernel CI to use qemu 8.2.0, and realized that
> >> thead c906 didn't boot anymore. Bisection points to commit d6a427e2c0b2
> >> ("target/riscv/cpu.c: restrict 'marchid' value")
> >>
> >> Reverting that commit, or the hack below solves the boot issue:
> >>
> >> --8<--
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 8cbfc7e781ad..e18596c8a55a 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -505,6 +505,9 @@ static void rv64_thead_c906_cpu_init(Object *obj)
> >> cpu->cfg.ext_xtheadsync = true;
> >>
> >> cpu->cfg.mvendorid = THEAD_VENDOR_ID;
> >> +cpu->cfg.marchid = ((QEMU_VERSION_MAJOR << 16) |
> >> +(QEMU_VERSION_MINOR << 8) |
> >> +(QEMU_VERSION_MICRO));
> >> #ifndef CONFIG_USER_ONLY
> >> set_satp_mode_max_supported(cpu, VM_1_10_SV39);
> >> #endif
> >> --8<--
> >>
> >> I'm unsure what the correct qemu way of adding a default value is,
> >> or if c906 should have a proper marchid.
> >
> > The "correct" marchid/mimpid values for the c906 are zero.
>
> Ok! Thanks for clearing that up for me.
>
> > I haven't looked into the code at all, so I am "assuming" that it is
> > being zero intialised at present. Linux applies the errata fixups for
> > the c906 when archid and impid are both zero - so your patch will avoid
> > these fixups being applied.
>
> I'm also assuming 0, -- will double-check. Hmm, that means that the
> *previous* marchid was incorrect (pre d6a427e2c0b2).
>
> > Do you think that perhaps the emulation in QEMU does not support what
> > the kernel uses once then errata fixups are enabled?
>
> Did a quick look at the c906 "in_asm,int" logs:
>
> | 0x80201040: 1273 sfence.vma zero,zero
> | 0x80201044: 18051073 csrrw zero,satp,a0
> |
> | riscv_cpu_do_interrupt: hart:0, async:0, cause:000c,
> epc:0x80201048, tval:0x80201048, desc=exec_page_fault
> | riscv_cpu_do_interrupt: hart:0, async:0, cause:000c,
> epc:0x80001048, tval:0x80001048, desc=exec_page_fault
> | ...cont forever
>
> So it looks like we're tripping over the page tables, when we're turning
> on paging.
>
> Hmm, maybe it's not qemu, but the c906 that has been broken for a while?
I didn't know what you mean by "not qemu, but the c906", so I went and
boot tested my d1 nezha. On today's next (6.8.0-rc1-next-20240124) it
booted into my initramfs with no problems. Obivously though my config is
unlikely to match yours, but that seems like a core thing that should be
hit regardless of config.
So perhaps this is a c906-in-QEMU problem? Lacking emulation for
something the kernel uses perhaps? I know nothing about the capabilities
of its emulation in QEMU, so I am of no help.
Cheers,
Conor.
>
> I'll disable it temporarily from CI anyhow, and will continue digging.
>
>
> Thanks for the pointers/clarifications, Conor!
> Björn
>
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
signature.asc
Description: PGP signature
Re: qemu riscv, thead c906, Linux boot regression
On Wed, Jan 24, 2024 at 01:49:51PM +0100, Björn Töpel wrote:
> Hi!
>
> I bumped the RISC-V Linux kernel CI to use qemu 8.2.0, and realized that
> thead c906 didn't boot anymore. Bisection points to commit d6a427e2c0b2
> ("target/riscv/cpu.c: restrict 'marchid' value")
>
> Reverting that commit, or the hack below solves the boot issue:
>
> --8<--
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8cbfc7e781ad..e18596c8a55a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -505,6 +505,9 @@ static void rv64_thead_c906_cpu_init(Object *obj)
> cpu->cfg.ext_xtheadsync = true;
>
> cpu->cfg.mvendorid = THEAD_VENDOR_ID;
> +cpu->cfg.marchid = ((QEMU_VERSION_MAJOR << 16) |
> +(QEMU_VERSION_MINOR << 8) |
> +(QEMU_VERSION_MICRO));
> #ifndef CONFIG_USER_ONLY
> set_satp_mode_max_supported(cpu, VM_1_10_SV39);
> #endif
> --8<--
>
> I'm unsure what the correct qemu way of adding a default value is,
> or if c906 should have a proper marchid.
The "correct" marchid/mimpid values for the c906 are zero.
I haven't looked into the code at all, so I am "assuming" that it is
being zero intialised at present. Linux applies the errata fixups for
the c906 when archid and impid are both zero - so your patch will avoid
these fixups being applied.
Do you think that perhaps the emulation in QEMU does not support what
the kernel uses once then errata fixups are enabled?
>
> Maybe Christoph or Zhiwei can answer?
>
> qemu command-line:
> qemu-system-riscv64 -nodefaults -nographic -machine virt,acpi=off \
>-cpu thead-c906 ...
>
>
> Thanks,
> Björn
>
signature.asc
Description: PGP signature
[PATCH v4 2/2] target/riscv: support new isa extension detection devicetree properties
From: Conor Dooley
A few months ago I submitted a patch to various lists, deprecating
"riscv,isa" with a lengthy commit message [0] that is now commit
aeb71e42caae ("dt-bindings: riscv: deprecate riscv,isa") in the Linux
kernel tree. Primarily, the goal was to replace "riscv,isa" with a new
set of properties that allowed for strictly defining the meaning of
various extensions, where "riscv,isa" was tied to whatever definitions
inflicted upon us by the ISA manual, which have seen some variance over
time.
Two new properties were introduced: "riscv,isa-base" and
"riscv,isa-extensions". The former is a simple string to communicate the
base ISA implemented by a hart and the latter an array of strings used
to communicate the set of ISA extensions supported, per the definitions
of each substring in extensions.yaml [1]. A beneficial side effect was
also the ability to define vendor extensions in a more "official" way,
as the ISA manual and other RVI specifications only covered the format
for vendor extensions in the ISA string, but not the meaning of vendor
extensions, for obvious reasons.
Add support for setting these two new properties in the devicetrees for
the various devicetree platforms supported by QEMU for RISC-V. The Linux
kernel already supports parsing ISA extensions from these new
properties, and documenting them in the dt-binding is a requirement for
new extension detection being added to the kernel.
A side effect of the implementation is that the meaning for elements in
"riscv,isa" and in "riscv,isa-extensions" are now tied together as they
are constructed from the same source. The same applies to the ISA string
provided in ACPI tables, but there does not appear to be any strict
definitions of meanings in ACPI land either.
Link:
https://lore.kernel.org/qemu-riscv/20230702-eats-scorebook-c951f170d29f@spud/
[0]
Link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/extensions.yaml
[1]
Reviewed-by: Alistair Francis
Reviewed-by: Andrew Jones
Signed-off-by: Conor Dooley
---
hw/riscv/sifive_u.c | 7 ++
hw/riscv/spike.c| 6 ++---
hw/riscv/virt.c | 6 ++---
target/riscv/cpu.c | 53 +
target/riscv/cpu.h | 1 +
5 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ec76dce6c9..2f227f15bc 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -171,7 +171,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
int cpu_phandle = phandle++;
nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
-char *isa;
qemu_fdt_add_subnode(fdt, nodename);
/* cpu 0 is the management hart that does not have mmu */
if (cpu != 0) {
@@ -180,11 +179,10 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
} else {
qemu_fdt_setprop_string(fdt, nodename, "mmu-type",
"riscv,sv48");
}
-isa = riscv_isa_string(>soc.u_cpus.harts[cpu - 1]);
+riscv_isa_write_fdt(>soc.u_cpus.harts[cpu - 1], fdt, nodename);
} else {
-isa = riscv_isa_string(>soc.e_cpus.harts[0]);
+riscv_isa_write_fdt(>soc.e_cpus.harts[0], fdt, nodename);
}
-qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
@@ -194,7 +192,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
-g_free(isa);
g_free(intc);
g_free(nodename);
}
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 81f7e53aed..64074395bc 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,7 +59,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry
*memmap,
MachineState *ms = MACHINE(s);
uint32_t *clint_cells;
uint32_t cpu_phandle, intc_phandle, phandle = 1;
-char *name, *mem_name, *clint_name, *clust_name;
+char *mem_name, *clint_name, *clust_name;
char *core_name, *cpu_name, *intc_name;
static const char * const clint_compat[2] = {
"sifive,clint0", "riscv,clint0"
@@ -113,9 +113,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry
*memmap,
} els
[PATCH v4 0/2] riscv: support new isa extension detection devicetree properties
From: Conor Dooley Making it a series to keep the standalone change to riscv_isa_string() that Drew reported separate. Changes in v4: - Other than a rebase, add a helper for the mxl_max to xlen conversion Changes in v3: - g_free() isa_extensions too - use misa_mxl_max rather than the compile target for the base isa - add a new patch changing riscv_isa_string() to do the same - drop a null check that cannot be null - rebased on top of Alistair's next branch Changes in v2: - use g_strdup() for multiletter extension string copying - wrap stuff in #ifndef to prevent breaking the user mode build - rename riscv_isa_set_props() -> riscv_isa_write_fdt() CC: Alistair Francis CC: Bin Meng CC: Palmer Dabbelt CC: Weiwei Li CC: Daniel Henrique Barboza CC: Andrew Jones CC: Liu Zhiwei CC: qemu-ri...@nongnu.org CC: qemu-devel@nongnu.org Conor Dooley (2): target/riscv: use misa_mxl_max to populate isa string rather than TARGET_LONG_BITS target/riscv: support new isa extension detection devicetree properties hw/riscv/sifive_u.c| 7 ++--- hw/riscv/spike.c | 6 ++-- hw/riscv/virt.c| 6 ++-- target/riscv/cpu.c | 62 +- target/riscv/cpu.h | 2 ++ target/riscv/gdbstub.c | 2 +- 6 files changed, 70 insertions(+), 15 deletions(-) -- 2.43.0
[PATCH v4 1/2] target/riscv: use misa_mxl_max to populate isa string rather than TARGET_LONG_BITS
From: Conor Dooley
A cpu may not have the same xlen as the compile time target, and
misa_mxl_max is the source of truth for what the hart supports.
The conversion from misa_mxl_max to xlen already has one user, so
introduce a helper and use that to populate the isa string.
Link: https://lore.kernel.org/qemu-riscv/20240108-efa3f83dcd3997dc0af458d7@orel/
Signed-off-by: Conor Dooley
---
I dropped the tags since I added the helper
---
target/riscv/cpu.c | 9 -
target/riscv/cpu.h | 1 +
target/riscv/gdbstub.c | 2 +-
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ad1df2318b..4aa4b2e988 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -307,6 +307,11 @@ void riscv_cpu_set_misa(CPURISCVState *env, RISCVMXL mxl,
uint32_t ext)
env->misa_ext_mask = env->misa_ext = ext;
}
+int riscv_cpu_max_xlen(CPURISCVState env)
+{
+return 16 << env.misa_mxl_max;
+}
+
#ifndef CONFIG_USER_ONLY
static uint8_t satp_mode_from_str(const char *satp_mode_str)
{
@@ -2332,7 +2337,9 @@ char *riscv_isa_string(RISCVCPU *cpu)
int i;
const size_t maxlen = sizeof("rv128") + sizeof(riscv_single_letter_exts);
char *isa_str = g_new(char, maxlen);
-char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
+int xlen = riscv_cpu_max_xlen(cpu->env);
+char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", xlen);
+
for (i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
*p++ = qemu_tolower(riscv_single_letter_exts[i]);
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 05e83c4ac9..aacc031397 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -511,6 +511,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
MMUAccessType access_type, int mmu_idx,
bool probe, uintptr_t retaddr);
char *riscv_isa_string(RISCVCPU *cpu);
+int riscv_cpu_max_xlen(CPURISCVState env);
bool riscv_cpu_option_set(const char *optname);
#define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 58b3ace0fe..f15980fdcf 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -218,7 +218,7 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int
base_reg)
CPURISCVState *env = >env;
GString *s = g_string_new(NULL);
riscv_csr_predicate_fn predicate;
-int bitsize = 16 << env->misa_mxl_max;
+int bitsize = riscv_cpu_max_xlen(*env);
int i;
#if !defined(CONFIG_USER_ONLY)
--
2.43.0
Re: [PATCH v3 0/2] riscv: support new isa extension detection devicetree properties
On Mon, Jan 22, 2024 at 03:24:19PM +1000, Alistair Francis wrote: > On Wed, Jan 10, 2024 at 8:27 PM Conor Dooley wrote: > > > > From: Conor Dooley > > > > Making it a series to keep the standalone change to riscv_isa_string() > > that Drew reported separate. > > > > Changes in v3: > > - g_free() isa_extensions too > > - use misa_mxl_max rather than the compile target for the base isa > > - add a new patch changing riscv_isa_string() to do the same > > - drop a null check that cannot be null > > - rebased on top of Alistair's next branch > > Do you mind rebasing on > https://github.com/alistair23/qemu/tree/riscv-to-apply.next again? > There was a big re-org recently so lots of rebasing is required I can, sure. Do you want me to introduce the macro that I mentioned in the first patch as a helper for misa_mxl_max -> width conversions when I do? Thanks, Conor. signature.asc Description: PGP signature
[PATCH v3 1/2] target/riscv: use misa_mxl_max to populate isa string rather than TARGET_LONG_BITS
From: Conor Dooley
A cpu may not have the same xlen as the compile time target, and
misa_mxl_max is the source of truth for what the hart supports.
Reported-by: Andrew Jones
Link: https://lore.kernel.org/qemu-riscv/20240108-efa3f83dcd3997dc0af458d7@orel/
Signed-off-by: Conor Dooley
---
Perhaps this misa_mxl_max -> width conversion should exist as a macro?
There's now 3 individual conversions of this type - two I added and one
in the gdb code.
---
target/riscv/cpu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8cbfc7e781..5b5da970f2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1860,7 +1860,9 @@ char *riscv_isa_string(RISCVCPU *cpu)
int i;
const size_t maxlen = sizeof("rv128") + sizeof(riscv_single_letter_exts);
char *isa_str = g_new(char, maxlen);
-char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
+int xlen = 16 << cpu->env.misa_mxl_max;
+char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", xlen);
+
for (i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
*p++ = qemu_tolower(riscv_single_letter_exts[i]);
--
2.39.2
[PATCH v3 0/2] riscv: support new isa extension detection devicetree properties
From: Conor Dooley Making it a series to keep the standalone change to riscv_isa_string() that Drew reported separate. Changes in v3: - g_free() isa_extensions too - use misa_mxl_max rather than the compile target for the base isa - add a new patch changing riscv_isa_string() to do the same - drop a null check that cannot be null - rebased on top of Alistair's next branch Changes in v2: - use g_strdup() for multiletter extension string copying - wrap stuff in #ifndef to prevent breaking the user mode build - rename riscv_isa_set_props() -> riscv_isa_write_fdt() CC: Alistair Francis CC: Bin Meng CC: Palmer Dabbelt CC: Weiwei Li CC: Daniel Henrique Barboza CC: Andrew Jones CC: Liu Zhiwei CC: qemu-ri...@nongnu.org CC: qemu-devel@nongnu.org Conor Dooley (2): target/riscv: use misa_mxl_max to populate isa string rather than TARGET_LONG_BITS target/riscv: support new isa extension detection devicetree properties hw/riscv/sifive_u.c | 7 ++ hw/riscv/spike.c| 6 ++--- hw/riscv/virt.c | 6 ++--- target/riscv/cpu.c | 57 - target/riscv/cpu.h | 1 + 5 files changed, 63 insertions(+), 14 deletions(-) -- 2.39.2
[PATCH v3 2/2] target/riscv: support new isa extension detection devicetree properties
From: Conor Dooley
A few months ago I submitted a patch to various lists, deprecating
"riscv,isa" with a lengthy commit message [0] that is now commit
aeb71e42caae ("dt-bindings: riscv: deprecate riscv,isa") in the Linux
kernel tree. Primarily, the goal was to replace "riscv,isa" with a new
set of properties that allowed for strictly defining the meaning of
various extensions, where "riscv,isa" was tied to whatever definitions
inflicted upon us by the ISA manual, which have seen some variance over
time.
Two new properties were introduced: "riscv,isa-base" and
"riscv,isa-extensions". The former is a simple string to communicate the
base ISA implemented by a hart and the latter an array of strings used
to communicate the set of ISA extensions supported, per the definitions
of each substring in extensions.yaml [1]. A beneficial side effect was
also the ability to define vendor extensions in a more "official" way,
as the ISA manual and other RVI specifications only covered the format
for vendor extensions in the ISA string, but not the meaning of vendor
extensions, for obvious reasons.
Add support for setting these two new properties in the devicetrees for
the various devicetree platforms supported by QEMU for RISC-V. The Linux
kernel already supports parsing ISA extensions from these new
properties, and documenting them in the dt-binding is a requirement for
new extension detection being added to the kernel.
A side effect of the implementation is that the meaning for elements in
"riscv,isa" and in "riscv,isa-extensions" are now tied together as they
are constructed from the same source. The same applies to the ISA string
provided in ACPI tables, but there does not appear to be any strict
definitions of meanings in ACPI land either.
Link:
https://lore.kernel.org/qemu-riscv/20230702-eats-scorebook-c951f170d29f@spud/
[0]
Link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/extensions.yaml
[1]
Signed-off-by: Conor Dooley
---
hw/riscv/sifive_u.c | 7 ++
hw/riscv/spike.c| 6 ++---
hw/riscv/virt.c | 6 ++---
target/riscv/cpu.c | 53 +
target/riscv/cpu.h | 1 +
5 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ec76dce6c9..2f227f15bc 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -171,7 +171,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
int cpu_phandle = phandle++;
nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
-char *isa;
qemu_fdt_add_subnode(fdt, nodename);
/* cpu 0 is the management hart that does not have mmu */
if (cpu != 0) {
@@ -180,11 +179,10 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
} else {
qemu_fdt_setprop_string(fdt, nodename, "mmu-type",
"riscv,sv48");
}
-isa = riscv_isa_string(>soc.u_cpus.harts[cpu - 1]);
+riscv_isa_write_fdt(>soc.u_cpus.harts[cpu - 1], fdt, nodename);
} else {
-isa = riscv_isa_string(>soc.e_cpus.harts[0]);
+riscv_isa_write_fdt(>soc.e_cpus.harts[0], fdt, nodename);
}
-qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
@@ -194,7 +192,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
-g_free(isa);
g_free(intc);
g_free(nodename);
}
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 81f7e53aed..64074395bc 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,7 +59,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry
*memmap,
MachineState *ms = MACHINE(s);
uint32_t *clint_cells;
uint32_t cpu_phandle, intc_phandle, phandle = 1;
-char *name, *mem_name, *clint_name, *clust_name;
+char *mem_name, *clint_name, *clust_name;
char *core_name, *cpu_name, *intc_name;
static const char * const clint_compat[2] = {
"sifive,clint0", "riscv,clint0"
@@ -113,9 +113,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry
*memmap,
} else {
qemu_fdt_setprop_string(fdt, cpu_name
[PATCH v2] riscv: support new isa extension detection devicetree properties
From: Conor Dooley
A few months ago I submitted a patch to various lists, deprecating
"riscv,isa" with a lengthy commit message [0] that is now commit
aeb71e42caae ("dt-bindings: riscv: deprecate riscv,isa") in the Linux
kernel tree. Primarily, the goal was to replace "riscv,isa" with a new
set of properties that allowed for strictly defining the meaning of
various extensions, where "riscv,isa" was tied to whatever definitions
inflicted upon us by the ISA manual, which have seen some variance over
time.
Two new properties were introduced: "riscv,isa-base" and
"riscv,isa-extensions". The former is a simple string to communicate the
base ISA implemented by a hart and the latter an array of strings used
to communicate the set of ISA extensions supported, per the definitions
of each substring in extensions.yaml [1]. A beneficial side effect was
also the ability to define vendor extensions in a more "official" way,
as the ISA manual and other RVI specifications only covered the format
for vendor extensions in the ISA string, but not the meaning of vendor
extensions, for obvious reasons.
Add support for setting these two new properties in the devicetrees for
the various devicetree platforms supported by QEMU for RISC-V. The Linux
kernel already supports parsing ISA extensions from these new
properties, and documenting them in the dt-binding is a requirement for
new extension detection being added to the kernel.
A side effect of the implementation is that the meaning for elements in
"riscv,isa" and in "riscv,isa-extensions" are now tied together as they
are constructed from the same source. The same applies to the ISA string
provided in ACPI tables, but there does not appear to be any strict
definitions of meanings in ACPI land either.
Link:
https://lore.kernel.org/qemu-riscv/20230702-eats-scorebook-c951f170d29f@spud/
[0]
Link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/extensions.yaml
[1]
Signed-off-by: Conor Dooley
---
Changes in v2:
- use g_strdup() for multiletter extension string copying
- wrap stuff in #ifndef to prevent breaking the user mode build
- rename riscv_isa_set_props() -> riscv_isa_write_fdt()
CC: Alistair Francis
CC: Bin Meng
CC: Palmer Dabbelt
CC: Weiwei Li
CC: Daniel Henrique Barboza
CC: Liu Zhiwei
CC: qemu-ri...@nongnu.org
CC: qemu-devel@nongnu.org
---
hw/riscv/sifive_u.c | 7 ++-
hw/riscv/spike.c| 6 ++
hw/riscv/virt.c | 6 ++
target/riscv/cpu.c | 50 +
target/riscv/cpu.h | 1 +
5 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ec76dce6c9..2f227f15bc 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -171,7 +171,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
int cpu_phandle = phandle++;
nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
-char *isa;
qemu_fdt_add_subnode(fdt, nodename);
/* cpu 0 is the management hart that does not have mmu */
if (cpu != 0) {
@@ -180,11 +179,10 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
} else {
qemu_fdt_setprop_string(fdt, nodename, "mmu-type",
"riscv,sv48");
}
-isa = riscv_isa_string(>soc.u_cpus.harts[cpu - 1]);
+riscv_isa_write_fdt(>soc.u_cpus.harts[cpu - 1], fdt, nodename);
} else {
-isa = riscv_isa_string(>soc.e_cpus.harts[0]);
+riscv_isa_write_fdt(>soc.e_cpus.harts[0], fdt, nodename);
}
-qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
@@ -194,7 +192,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
-g_free(isa);
g_free(intc);
g_free(nodename);
}
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 81f7e53aed..64074395bc 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,7 +59,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry
*memmap,
MachineState *ms = MACHINE(s);
uint32_t *clint_cells;
uint32_t cpu_phandle, intc_phandle, phandle = 1;
-char *name, *mem_name, *clint_name, *clust_n
Re: [PATCH v1] riscv: support new isa extension detection devicetree properties
On Wed, Nov 29, 2023 at 11:37:00AM -0300, Daniel Henrique Barboza wrote:
> On 11/29/23 07:39, Conor Dooley wrote:
> > From: Conor Dooley
> >
> > A few months ago I submitted a patch to various lists, deprecating
> > "riscv,isa" with a lengthy commit message [0] that is now commit
> > aeb71e42caae ("dt-bindings: riscv: deprecate riscv,isa") in the Linux
> > kernel tree. Primarily, the goal was to replace "riscv,isa" with a new
> > set of properties that allowed for strictly defining the meaning of
> > various extensions, where "riscv,isa" was tied to whatever definitions
> > inflicted upon us by the ISA manual, which have seen some variance over
> > time.
> >
> > Two new properties were introduced: "riscv,isa-base" and
> > "riscv,isa-extensions". The former is a simple string to communicate the
> > base ISA implemented by a hart and the latter an array of strings used
> > to communicate the set of ISA extensions supported, per the definitions
> > of each substring in extensions.yaml [1]. A beneficial side effect was
> > also the ability to define vendor extensions in a more "official" way,
> > as the ISA manual and other RVI specifications only covered the format
> > for vendor extensions in the ISA string, but not the meaning of vendor
> > extensions, for obvious reasons.
> >
> > Add support for setting these two new properties in the devicetrees for
> > the various devicetree platforms supported by QEMU for RISC-V. The Linux
> > kernel already supports parsing ISA extensions from these new
> > properties, and documenting them in the dt-binding is a requirement for
> > new extension detection being added to the kernel.
> >
> > A side effect of the implementation is that the meaning for elements in
> > "riscv,isa" and in "riscv,isa-extensions" are now tied together as they
> > are constructed from the same source. The same applies to the ISA string
> > provided in ACPI tables, but there does not appear to be any strict
> > definitions of meanings in ACPI land either.
> >
> > Link:
> > https://lore.kernel.org/qemu-riscv/20230702-eats-scorebook-c951f170d29f@spud/
> > [0]
> > Link:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/extensions.yaml
> > [1]
> > Signed-off-by: Conor Dooley
> > ---
>
> What about new extensions? Do we still need them to be present in riscv,isa
> or can we just add them to riscv,isa-extensions? Eventually we'll want
> applications
> to move on and stop using riscv,isa altogether, but if we keep adding new
> extensions to it this process will take longer.
Yeah, I kinda didn't want to go there any time soon though, since this
is only the second codebase I've added the properties to so far. If
preventing adding new things to "riscv,isa" is something the maintainers
of the RISC-V QEMU support are down to do, I certainly would not object.
>
> > The apart from the bonding of ACPI and DT definitions which concerns me
> > a bit, I'm a wee bit worried about the vendor extensions diverging from
> > what ends up in bindings. Ideally I think there should be acked binding
> > patches for extensions, but that's well outside of my jurisdiction!
> >
> > There's two 80 char line length violations in this, but the file already
> > has some > 80 char lines, so I figured that it'd be fine...
>
> It's fine to me.
Cool.
> > +void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename)
> > +{
> > +const size_t maxlen = sizeof("rv128i");
> > +g_autofree char *isa_base = g_new(char, maxlen);
> > +g_autofree char *riscv_isa;
> > +char **isa_extensions;
> > +int count = 0;
> > +
> > +riscv_isa = riscv_isa_string(cpu);
> > +qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", riscv_isa);
> > +
> > +snprintf(isa_base, maxlen, "rv%di", TARGET_LONG_BITS);
> > +qemu_fdt_setprop_string(fdt, nodename, "riscv,isa-base", isa_base);
> > +
> > +isa_extensions = riscv_isa_extensions_list(cpu, );
> > +qemu_fdt_setprop_string_array(fdt, nodename, "riscv,isa-extensions",
> > + isa_extensions, count);
> > +
> > +for (int i = 0; i < count; i++) {
> > +g_free(isa_extensions[i]);
> > +}
> > +}
>
> This will break user-mode build:
>
> [2025/2626] Linking target qemu-riscv64
> FAILED: qemu-riscv64
> (...)
> /usr/bin/ld:
[PATCH v1] riscv: support new isa extension detection devicetree properties
From: Conor Dooley
A few months ago I submitted a patch to various lists, deprecating
"riscv,isa" with a lengthy commit message [0] that is now commit
aeb71e42caae ("dt-bindings: riscv: deprecate riscv,isa") in the Linux
kernel tree. Primarily, the goal was to replace "riscv,isa" with a new
set of properties that allowed for strictly defining the meaning of
various extensions, where "riscv,isa" was tied to whatever definitions
inflicted upon us by the ISA manual, which have seen some variance over
time.
Two new properties were introduced: "riscv,isa-base" and
"riscv,isa-extensions". The former is a simple string to communicate the
base ISA implemented by a hart and the latter an array of strings used
to communicate the set of ISA extensions supported, per the definitions
of each substring in extensions.yaml [1]. A beneficial side effect was
also the ability to define vendor extensions in a more "official" way,
as the ISA manual and other RVI specifications only covered the format
for vendor extensions in the ISA string, but not the meaning of vendor
extensions, for obvious reasons.
Add support for setting these two new properties in the devicetrees for
the various devicetree platforms supported by QEMU for RISC-V. The Linux
kernel already supports parsing ISA extensions from these new
properties, and documenting them in the dt-binding is a requirement for
new extension detection being added to the kernel.
A side effect of the implementation is that the meaning for elements in
"riscv,isa" and in "riscv,isa-extensions" are now tied together as they
are constructed from the same source. The same applies to the ISA string
provided in ACPI tables, but there does not appear to be any strict
definitions of meanings in ACPI land either.
Link:
https://lore.kernel.org/qemu-riscv/20230702-eats-scorebook-c951f170d29f@spud/
[0]
Link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/extensions.yaml
[1]
Signed-off-by: Conor Dooley
---
The apart from the bonding of ACPI and DT definitions which concerns me
a bit, I'm a wee bit worried about the vendor extensions diverging from
what ends up in bindings. Ideally I think there should be acked binding
patches for extensions, but that's well outside of my jurisdiction!
There's two 80 char line length violations in this, but the file already
has some > 80 char lines, so I figured that it'd be fine...
CC: Alistair Francis
CC: Bin Meng
CC: Palmer Dabbelt
CC: Weiwei Li
CC: Daniel Henrique Barboza
CC: Liu Zhiwei
CC: qemu-ri...@nongnu.org
CC: qemu-devel@nongnu.org
---
hw/riscv/sifive_u.c | 7 ++-
hw/riscv/spike.c| 6 ++
hw/riscv/virt.c | 6 ++
target/riscv/cpu.c | 50 +
target/riscv/cpu.h | 1 +
5 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ec76dce6c9..4e6eed863b 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -171,7 +171,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
int cpu_phandle = phandle++;
nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
-char *isa;
qemu_fdt_add_subnode(fdt, nodename);
/* cpu 0 is the management hart that does not have mmu */
if (cpu != 0) {
@@ -180,11 +179,10 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
} else {
qemu_fdt_setprop_string(fdt, nodename, "mmu-type",
"riscv,sv48");
}
-isa = riscv_isa_string(>soc.u_cpus.harts[cpu - 1]);
+riscv_isa_set_props(>soc.u_cpus.harts[cpu - 1], fdt, nodename);
} else {
-isa = riscv_isa_string(>soc.e_cpus.harts[0]);
+riscv_isa_set_props(>soc.e_cpus.harts[0], fdt, nodename);
}
-qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
@@ -194,7 +192,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry
*memmap,
qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
-g_free(isa);
g_free(intc);
g_free(nodename);
}
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 81f7e53aed..1657554d7b 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,7 +59,7 @@ st
Re: [PATCH for-8.2 v5 09/11] target/riscv: add 'max' CPU type
On Thu, Jul 27, 2023 at 11:12:34AM -0300, Daniel Henrique Barboza wrote: > > > On 7/27/23 10:59, Conor Dooley wrote: > > Hey Daniel, > > > > On Thu, Jul 20, 2023 at 02:19:31PM -0300, Daniel Henrique Barboza wrote: > > > The 'max' CPU type is used by tooling to determine what's the most > > > capable CPU a current QEMU version implements. Other archs such as ARM > > > implements this type. Let's add it to RISC-V. > > > > > > What we consider "most capable CPU" in this context are related to > > > ratified, non-vendor extensions. This means that we want the 'max' CPU > > > to enable all (possible) ratified extensions by default. The reasoning > > > behind this design is (1) vendor extensions can conflict with each other > > > and we won't play favorities deciding which one is default or not and > > > (2) non-ratified extensions are always prone to changes, not being > > > stable enough to be enabled by default. > > > > > > All this said, we're still not able to enable all ratified extensions > > > due to conflicts between them. Zfinx and all its dependencies aren't > > > enabled because of a conflict with RVF. zce, zcmp and zcmt are also > > > disabled due to RVD conflicts. When running with 64 bits we're also > > > disabling zcf. > > > > > > MISA bits RVG, RVJ and RVV are also being set manually since they're > > > default disabled. > > > > > > This is the resulting 'riscv,isa' DT for this new CPU: > > > > > > rv64imafdcvh_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_ > > > zfh_zfhmin_zca_zcb_zcd_zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_ > > > zkne_zknh_zkr_zks_zksed_zksh_zkt_zve32f_zve64f_zve64d_ > > > smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt > > > > > > Signed-off-by: Daniel Henrique Barboza > > > Reviewed-by: Weiwei Li > > > > I was giving this another go today, like so > > $(qemu) -smp 4 -M virt,aia=aplic,dumpdtb=qemu.dtb -cpu max -m 1G > > which lead to a few > > vector version is not specified, use the default value v1.0 > > printed. Should the max cpu set a vector version w/o user input > > being required? > > > This isn't exclusive to the 'max' cpu code per se. It's the common RVV > handling > code that is expecting users to inform which vector version they're going to > use every time we activate V. Yah, I figured it was not exclusive to it, but it seemed "thematic" for the max cpu to silently pick a reasonable default rather than complain. > I believe it's too late to change the command line handling to force users to > pick > a vector version, so the second better approach is to silently default to v1.0 > (or perhaps to the latest RVV version available) if the user didn't set > anything. Honestly, I'm not sure why it warns at the moment. Seems like the default is what any reasonable person would expect, no? signature.asc Description: PGP signature
[PATCH] hw/riscv: virt: Fix riscv,pmu DT node path
From: Conor Dooley
On a dtb dumped from the virt machine, dt-validate complains:
soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284],
[65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280]],
'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}
from schema $id: http://devicetree.org/schemas/simple-bus.yaml#
That's pretty cryptic, but running the dtb back through dtc produces
something a lot more reasonable:
Warning (simple_bus_reg): /soc/pmu: missing or empty reg/ranges property
Moving the riscv,pmu node out of the soc bus solves the problem.
Signed-off-by: Conor Dooley
---
CC: Palmer Dabbelt
CC: Alistair Francis
CC: Bin Meng
CC: Weiwei Li
CC: Daniel Henrique Barboza
CC: Liu Zhiwei
CC: qemu-ri...@nongnu.org
CC: qemu-devel@nongnu.org
---
hw/riscv/virt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d90286dc46..25dcc2616e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -732,7 +732,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
MachineState *ms = MACHINE(s);
RISCVCPU hart = s->soc[0].harts[0];
-pmu_name = g_strdup_printf("/soc/pmu");
+pmu_name = g_strdup_printf("/pmu");
qemu_fdt_add_subnode(ms->fdt, pmu_name);
qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
--
2.39.2
Re: [PATCH for-8.2 v5 09/11] target/riscv: add 'max' CPU type
Hey Daniel, On Thu, Jul 20, 2023 at 02:19:31PM -0300, Daniel Henrique Barboza wrote: > The 'max' CPU type is used by tooling to determine what's the most > capable CPU a current QEMU version implements. Other archs such as ARM > implements this type. Let's add it to RISC-V. > > What we consider "most capable CPU" in this context are related to > ratified, non-vendor extensions. This means that we want the 'max' CPU > to enable all (possible) ratified extensions by default. The reasoning > behind this design is (1) vendor extensions can conflict with each other > and we won't play favorities deciding which one is default or not and > (2) non-ratified extensions are always prone to changes, not being > stable enough to be enabled by default. > > All this said, we're still not able to enable all ratified extensions > due to conflicts between them. Zfinx and all its dependencies aren't > enabled because of a conflict with RVF. zce, zcmp and zcmt are also > disabled due to RVD conflicts. When running with 64 bits we're also > disabling zcf. > > MISA bits RVG, RVJ and RVV are also being set manually since they're > default disabled. > > This is the resulting 'riscv,isa' DT for this new CPU: > > rv64imafdcvh_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_ > zfh_zfhmin_zca_zcb_zcd_zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_ > zkne_zknh_zkr_zks_zksed_zksh_zkt_zve32f_zve64f_zve64d_ > smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt > > Signed-off-by: Daniel Henrique Barboza > Reviewed-by: Weiwei Li I was giving this another go today, like so $(qemu) -smp 4 -M virt,aia=aplic,dumpdtb=qemu.dtb -cpu max -m 1G which lead to a few vector version is not specified, use the default value v1.0 printed. Should the max cpu set a vector version w/o user input being required? Cheers, Conor. signature.asc Description: PGP signature
Re: [PATCH] roms/opensbi: Upgrade from v1.3 to v1.3.1
On Fri, Jul 21, 2023 at 10:04:02AM +1000, Alistair Francis wrote: > On Thu, Jul 20, 2023 at 3:00 AM Bin Meng wrote: > > > > Upgrade OpenSBI from v1.3 to v1.3.1 and the pre-built bios images > > which fixes the boot failure seen when using QEMU to do a direct > > kernel boot with Microchip Icicle Kit board machine. > > > > The v1.3.1 release includes the following commits: > > > > 0907de3 lib: sbi: fix comment indent > > eb736a5 lib: sbi_pmu: Avoid out of bounds access > > 7828eeb gpio/desginware: add Synopsys DesignWare APB GPIO support > > c6a3573 lib: utils: Fix sbi_hartid_to_scratch() usage in ACLINT drivers > > 057eb10 lib: utils/gpio: Fix RV32 compile error for designware GPIO driver > > > > Signed-off-by: Bin Meng > > Reviewed-by: Alistair Francis > > @Conor Dooley @Conor Dooley Any chance you can test this? Sure. I didn't se this patch because I am not subscribed to qemu-devel, just qemu-riscv. Tested-by: Conor Dooley Thanks, Conor. signature.asc Description: PGP signature
Re: Boot failure after QEMU's upgrade to OpenSBI v1.3 (was Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type)
On Wed, Jul 19, 2023 at 11:32:55AM +1000, Alistair Francis wrote:
> If there is no OpenSBI 1.3.1 release we should add something to the
> release notes. @Conor Dooley are you able to give a clear sentence on
> how the boot fails?
Uhh, I'll give it a shot, but hopefully it is not required :)
In version v1.3, OpenSBI's aclint drivers fail to initialise if they
encounter a disabled CPU node in the devicetree. Attempting to boot
using, for example, the Linux kernel's PolarFire SoC or Freedom U540
devicetrees, will fail with the error:
"init_coldboot: ipi init failed (error -1009)"
Please see OpenSBI commit c6a3573 ("lib: utils: Fix sbi_hartid_to_scratch()
usage in ACLINT drivers")
<https://github.com/riscv-software-src/opensbi/commit/c6a35733b74aeff612398f274ed19a74f81d1f37>
for the fix.
signature.asc
Description: PGP signature
Re: [PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags
On Mon, Jul 17, 2023 at 08:11:09PM -0300, Daniel Henrique Barboza wrote: > On 7/17/23 19:33, Conor Dooley wrote: > > On Mon, Jul 17, 2023 at 06:54:17PM -0300, Daniel Henrique Barboza wrote: > > > Hi, > > > > > > I decided to include flags for both timer/counter extensions to make it > > > easier for us later on when dealing with the RVA22 profile (which > > > includes both). > > > > > > The features were already implemented by Atish Patra some time ago, but > > > back then these 2 extensions weren't introduced yet. This means that, > > > aside from extra stuff in riscv,isa FDT no other functional changes were > > > made. > > > > > > Both are defaulted to 'true' since QEMU already implements both > > > features, but the flag can be disabled if Zicsr isn't present or, in > > > the case of zihpm, if pmu_num = 0. > > > > Out of curiosity, since you are allowing them to be disabled, how do you > > intend to communicate to a guest that zicsr or zihpm are not present? > > At this point I'd say that existing guests are using other ways of checking > if these timers and counters are available. Or they just assume they're there as part of their baseline requirements ;) > After this patches OSes can confirm > if these timers are available via riscv,isa, but they can't assume that > they are not available if riscv,isa doesn't display them. > > There's a chance that guests will continue ignoring these 2 extensions > regardless > of whether the platform exposes them or not. > > > > > > This means that, > > > aside from extra stuff in riscv,isa FDT no other functional changes were > > > made. > > > > This is barely a "functional" change either, as the presence of these > > extensions has to be assumed, whether they appear in riscv,isa or not :/ > > It's more of an organizational change for the sake of QEMU internals because > the > RVA22 profile happens to include zicntr and zihpm as mandatory extensions. > It's > easier to add the flags than to document why we're claiming RVA22 support but > aren't displaying these 2 in riscv,isa. Possibly you should call out ACPI here too, since that does not suffer from the same issues as riscv,isa in DT, and putting zicntr/zihpm et al in the ISA string there is needed. signature.asc Description: PGP signature
Re: [PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags
Hey, On Mon, Jul 17, 2023 at 06:54:17PM -0300, Daniel Henrique Barboza wrote: > Hi, > > I decided to include flags for both timer/counter extensions to make it > easier for us later on when dealing with the RVA22 profile (which > includes both). > > The features were already implemented by Atish Patra some time ago, but > back then these 2 extensions weren't introduced yet. This means that, > aside from extra stuff in riscv,isa FDT no other functional changes were > made. > > Both are defaulted to 'true' since QEMU already implements both > features, but the flag can be disabled if Zicsr isn't present or, in > the case of zihpm, if pmu_num = 0. Out of curiosity, since you are allowing them to be disabled, how do you intend to communicate to a guest that zicsr or zihpm are not present? > This means that, > aside from extra stuff in riscv,isa FDT no other functional changes were > made. This is barely a "functional" change either, as the presence of these extensions has to be assumed, whether they appear in riscv,isa or not :/ signature.asc Description: PGP signature
Re: [PATCH] target/riscv/cpu.c: check priv_ver before auto-enable zca/zcd/zcf
On Mon, Jul 17, 2023 at 12:41:41PM -0300, Daniel Henrique Barboza wrote: > Commit bd30559568 made changes in how we're checking and disabling > extensions based on env->priv_ver. One of the changes was to move the > extension disablement code to the end of realize(), being able to > disable extensions after we've auto-enabled some of them. > > An unfortunate side effect of this change started to happen with CPUs > that has an older priv version, like sifive-u54. Starting on commit > 2288a5ce43e5 we're auto-enabling zca, zcd and zcf if RVC is enabled, > but these extensions are priv version 1.12.0. When running a cpu that > has an older priv ver (like sifive-u54) the user is spammed with > warnings like these: > > qemu-system-riscv64: warning: disabling zca extension for hart > 0x because privilege spec version does not match > qemu-system-riscv64: warning: disabling zcd extension for hart > 0x because privilege spec version does not match > > The warnings are part of the code that disables the extension, but in this > case we're throwing user warnings for stuff that we enabled on our own, > without user intervention. Users are left wondering what they did wrong. > > A quick 8.1 fix for this nuisance is to check the CPU priv spec before > auto-enabling zca/zcd/zcf. A more appropriate fix will include a more > robust framework that will account for both priv_ver and user choice > when auto-enabling/disabling extensions, but for 8.1 we'll make it do > with this simple check. > > It's also worth noticing that this is the only case where we're > auto-enabling extensions based on a criteria (in this case RVC) that > doesn't match the priv spec of the extensions we're enabling. There's no > need for more 8.1 band-aids. > > Cc: Conor Dooley Does the job, thanks for doing this. Tested-by: Conor Dooley Cheers, Conor. signature.asc Description: PGP signature
Re: Boot failure after QEMU's upgrade to OpenSBI v1.3 (was Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type)
On Fri, Jul 14, 2023 at 11:19:34AM +0100, Conor Dooley wrote:
> On Fri, Jul 14, 2023 at 10:00:19AM +0530, Anup Patel wrote:
>
> > > > OpenSBI v1.3
> > > >_ _
> > > > / __ \ / | _ \_ _|
> > > > | | | |_ __ ___ _ __ | (___ | |_) || |
> > > > | | | | '_ \ / _ \ '_ \ \___ \| _ < | |
> > > > | |__| | |_) | __/ | | |) | |_) || |_
> > > > \/| .__/ \___|_| |_|_/|___/_|
> > > > | |
> > > > |_|
> > > >
> > > > init_coldboot: ipi init failed (error -1009)
> > > >
> > > > Just to note, because we use our own firmware that vendors in OpenSBI
> > > > and compiles only a significantly cut down number of files from it, we
> > > > do not use the fw_dynamic etc flow on our hardware. As a result, we have
> > > > not tested v1.3, nor do we have any immediate plans to change our
> > > > platform firmware to vendor v1.3 either.
> > > >
> > > > I unless there's something obvious to you, it sounds like I will need to
> > > > go and bisect OpenSBI. That's a job for another day though, given the
> > > > time.
> > > >
> >
> > The real issue is some CPU/HART DT nodes marked as disabled in the
> > DT passed to OpenSBI 1.3.
> >
> > This issue does not exist in any of the DTs generated by QEMU but some
> > of the DTs in the kernel (such as microchip and SiFive board DTs) have
> > the E-core disabled.
> >
> > I had discovered this issue in a totally different context after the
> > OpenSBI 1.3
> > release happened. This issue is already fixed in the latest OpenSBI by the
> > following commit c6a35733b74aeff612398f274ed19a74f81d1f37 ("lib: utils:
> > Fix sbi_hartid_to_scratch() usage in ACLINT drivers").
>
> Great, thanks Anup! I thought I had tested tip-of-tree too, but
> obviously not.
>
> > I always assumed that Microchip hss.bin is the preferred BIOS for the
> > QEMU microchip-icicle-kit machine but I guess that's not true.
>
> Unfortunately the HSS has not worked in QEMU for a long time, and while
> I would love to fix it, but am pretty stretched for spare time to begin
> with.
> I usually just do direct kernel boots, which use the OpenSBI that comes
> with QEMU, as I am sure you already know :)
>
> > At this point, you can either:
> > 1) Use latest OpenSBI on QEMU microchip-icicle-kit machine
I forgot to reply to this point, wondering what should be done with
QEMU. Bumping to v1.3 in QEMU introduces a regression here, regardless
of whether I can go and build a fixed version of OpenSBI.
> > 2) Ensure CPU0 DT node is enabled in DT when booting on QEMU
> > microchip-icicle-kit machine with OpenSBI 1.3
>
> Will OpenSBI disable it? If not, I think option 2) needs to be remove
> the DT node. I'll just use tip-of-tree myself & up to the
Clearly didn't finish this comment. It was meant to say "up to the QEMU
maintainers what they want to do on the QEMU side of things".
Thanks,
Conor.
signature.asc
Description: PGP signature
Re: Boot failure after QEMU's upgrade to OpenSBI v1.3 (was Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type)
On Fri, Jul 14, 2023 at 10:00:19AM +0530, Anup Patel wrote:
> > > OpenSBI v1.3
> > >_ _
> > > / __ \ / | _ \_ _|
> > > | | | |_ __ ___ _ __ | (___ | |_) || |
> > > | | | | '_ \ / _ \ '_ \ \___ \| _ < | |
> > > | |__| | |_) | __/ | | |) | |_) || |_
> > > \/| .__/ \___|_| |_|_/|___/_|
> > > | |
> > > |_|
> > >
> > > init_coldboot: ipi init failed (error -1009)
> > >
> > > Just to note, because we use our own firmware that vendors in OpenSBI
> > > and compiles only a significantly cut down number of files from it, we
> > > do not use the fw_dynamic etc flow on our hardware. As a result, we have
> > > not tested v1.3, nor do we have any immediate plans to change our
> > > platform firmware to vendor v1.3 either.
> > >
> > > I unless there's something obvious to you, it sounds like I will need to
> > > go and bisect OpenSBI. That's a job for another day though, given the
> > > time.
> > >
>
> The real issue is some CPU/HART DT nodes marked as disabled in the
> DT passed to OpenSBI 1.3.
>
> This issue does not exist in any of the DTs generated by QEMU but some
> of the DTs in the kernel (such as microchip and SiFive board DTs) have
> the E-core disabled.
>
> I had discovered this issue in a totally different context after the OpenSBI
> 1.3
> release happened. This issue is already fixed in the latest OpenSBI by the
> following commit c6a35733b74aeff612398f274ed19a74f81d1f37 ("lib: utils:
> Fix sbi_hartid_to_scratch() usage in ACLINT drivers").
Great, thanks Anup! I thought I had tested tip-of-tree too, but
obviously not.
> I always assumed that Microchip hss.bin is the preferred BIOS for the
> QEMU microchip-icicle-kit machine but I guess that's not true.
Unfortunately the HSS has not worked in QEMU for a long time, and while
I would love to fix it, but am pretty stretched for spare time to begin
with.
I usually just do direct kernel boots, which use the OpenSBI that comes
with QEMU, as I am sure you already know :)
> At this point, you can either:
> 1) Use latest OpenSBI on QEMU microchip-icicle-kit machine
> 2) Ensure CPU0 DT node is enabled in DT when booting on QEMU
> microchip-icicle-kit machine with OpenSBI 1.3
Will OpenSBI disable it? If not, I think option 2) needs to be remove
the DT node. I'll just use tip-of-tree myself & up to the
signature.asc
Description: PGP signature
Re: Boot failure after QEMU's upgrade to OpenSBI v1.3 (was Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type)
On Thu, Jul 13, 2023 at 11:12:33PM +0100, Conor Dooley wrote: > +CC OpenSBI Mailing list > > I've not yet had the chance to bisect this, so adding the OpenSBI folks > to CC in case they might have an idea for what to try. NVM this, I bisected it. Logs below. > And a question for you below Daniel. > > On Wed, Jul 12, 2023 at 11:14:21PM +0100, Conor Dooley wrote: > > On Wed, Jul 12, 2023 at 06:39:28PM -0300, Daniel Henrique Barboza wrote: > > > On 7/12/23 18:35, Conor Dooley wrote: > > > > On Wed, Jul 12, 2023 at 06:09:10PM -0300, Daniel Henrique Barboza wrote: > > > > > > > > > It is intentional. Those default marchid/mimpid vals were derived > > > > > from the current > > > > > QEMU version ID/build and didn't mean much. > > > > > > > > > > It is still possible to set them via "-cpu rv64,marchid=N,mimpid=N" > > > > > if needed when > > > > > using the generic (rv64,rv32) CPUs. Vendor CPUs can't have their > > > > > machine IDs changed > > > > > via command line. > > > > > > > > Sounds good, thanks. I did just now go and check icicle to see what it > > > > would report & it does not boot. I'll go bisect... > > > > > > BTW how are you booting the icicle board nowadays? I remember you > > > mentioning about > > > some changes in the FDT being required to boot and whatnot. > > > > I do direct kernel boots, as the HSS doesn't work anymore, and just lie > > a bit to QEMU about how much DDR we have. > > .PHONY: qemu-icicle > > qemu-icicle: > > $(qemu) -M microchip-icicle-kit \ > > -m 3G -smp 5 \ > > -kernel $(vmlinux_bin) \ > > -dtb $(icicle_dtb) \ > > -initrd $(initramfs) \ > > -display none -serial null \ > > -serial stdio \ > > -D qemu.log -d unimp > > > > The platform only supports 2 GiB of DDR, not 3, but if I pass 2 to QEMU > > it thinks there's 1 GiB at 0x8000_ and 1 GiB at 0x10__. The > > upstream devicetree (and current FPGA reference design) expects there to > > be 1 GiB at 0x8000_ and 1 GiB at 0x10_4000_. If I lie to QEMU, > > it thinks there is 1 GiB at 0x8000_ and 2 GiB at 0x10__, and > > things just work. I prefer doing it this way than having to modify the > > DT, it is a lot easier to explain to people this way. > > > > I've been meaning to work the support for the icicle & mpfs in QEMU, but > > it just gets shunted down the priority list. I'd really like if a proper > > boot flow would run in QEMU, which means fixing whatever broke the HSS, > > but I've recently picked up maintainership of dt-binding stuff in Linux, > > so I've unfortunately got even less time to try and work on it. Maybe > > we'll get some new graduate in and I can make them suffer in my stead... > > > > > If it's not too hard I'll add it in my test scripts to keep it under > > > check. Perhaps > > > we can even add it to QEMU testsuite. > > > > I don't think it really should be that bad, at least for the direct > > kernel boot, which is what I mainly care about, since I use it fairly > > often for debugging boot stuff in Linux. > > > > Anyways, aa903cf31391dd505b399627158f1292a6d19896 is the first bad commit: > > commit aa903cf31391dd505b399627158f1292a6d19896 > > Author: Bin Meng > > Date: Fri Jun 30 23:36:04 2023 +0800 > > > > roms/opensbi: Upgrade from v1.2 to v1.3 > > > > Upgrade OpenSBI from v1.2 to v1.3 and the pre-built bios images. > > > > And I see something like: > > qemu//build/qemu-system-riscv64 -M microchip-icicle-kit \ > > -m 3G -smp 5 \ > > -kernel vmlinux.bin \ > > -dtb icicle.dtb \ > > -initrd initramfs.cpio.gz \ > > -display none -serial null \ > > -serial stdio \ > > -D qemu.log -d unimp > > > qemu-system-riscv64: warning: disabling zca extension for hart > > 0x because privilege spec version does not match > > qemu-system-riscv64: warning: disabling zca extension for hart > > 0x0001 because privilege spec version does not match > > qemu-system-riscv64: warning: disabling zcd extension for hart > > 0x0001 because privilege spec version does not match > > qemu-system-riscv64: warning: disabling zca extension for hart > > 0x0002 because privilege spec version does not match
Re: Boot failure after QEMU's upgrade to OpenSBI v1.3 (was Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type)
On Thu, Jul 13, 2023 at 07:35:01PM -0300, Daniel Henrique Barboza wrote: > On 7/13/23 19:12, Conor Dooley wrote: > > And a question for you below Daniel. > > > > On Wed, Jul 12, 2023 at 11:14:21PM +0100, Conor Dooley wrote: > > > > > qemu-system-riscv64: warning: disabling zca extension for hart > > > 0x because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zca extension for hart > > > 0x0001 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zcd extension for hart > > > 0x0001 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zca extension for hart > > > 0x0002 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zcd extension for hart > > > 0x0002 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zca extension for hart > > > 0x0003 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zcd extension for hart > > > 0x0003 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zca extension for hart > > > 0x0004 because privilege spec version does not match > > > qemu-system-riscv64: warning: disabling zcd extension for hart > > > 0x0004 because privilege spec version does not match > > > > Why am I seeing these warnings? Does the mpfs machine type need to > > disable some things? It only supports rv64imafdc per the DT, and > > predates things like Zca existing, so emitting warnings does not seem > > fair at all to me! > > QEMU will disable extensions that are newer than a priv spec version that is > set > by the CPU. IIUC the icicle board is running a sifive_u54 CPU by default. That > CPU has a priv spec version 1_10_0. The CPU is also enabling C. > > We will enable zca if C is enabled. C and D enabled will also enable zcd. But > then the priv check will disabled both because zca and zcd have priv spec > 1_12_0. > > This is a side effect for a change that I did a few months ago. Back then we > weren't disabling stuff correctly. Yah, I did check out the blame, hence directing it at you. Thanks for the explanation. > The warnings are annoying but are benign. To be honest, benign or not, this is kind of thing is only going to lead to grief. Even though only the direct kernel boot works, we do actually have some customers that are using the icicle target in QEMU. > And apparently the sifive_u54 CPU is being inconsistent for some time and > we noticed just now. > Now, if the icicle board is supposed to have zca and zcd then we have a > problem. I don't know, this depends on how you see things in QEMU. I would say that it supports c, and not Zca/Zcf/Zcd, given it predates the extensions. I have no interest in retrofitting my devicetree stuff with them, for example. > We'll need to discuss whether we move sifive_u54 CPU priv spec to 1_12_0 (I'm > not > sure how this will affect other boards that uses this CPU) or remove this > priv spec > disable code altogether from QEMU. I think you should stop warning for this? From my dumb-user perspective, the warning only "scares" me into thinking something is wrong, when there isn't. I can see a use case for the warning where someone tries to enable Zca & Co. in their QEMU incantation for a CPU that does not have the correct privilege level to support it, but I didn't try to set any options at all in that way, so the warnings seem unfair? Cheers, Conor. signature.asc Description: PGP signature
Boot failure after QEMU's upgrade to OpenSBI v1.3 (was Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type)
+CC OpenSBI Mailing list I've not yet had the chance to bisect this, so adding the OpenSBI folks to CC in case they might have an idea for what to try. And a question for you below Daniel. On Wed, Jul 12, 2023 at 11:14:21PM +0100, Conor Dooley wrote: > On Wed, Jul 12, 2023 at 06:39:28PM -0300, Daniel Henrique Barboza wrote: > > On 7/12/23 18:35, Conor Dooley wrote: > > > On Wed, Jul 12, 2023 at 06:09:10PM -0300, Daniel Henrique Barboza wrote: > > > > > > > It is intentional. Those default marchid/mimpid vals were derived from > > > > the current > > > > QEMU version ID/build and didn't mean much. > > > > > > > > It is still possible to set them via "-cpu rv64,marchid=N,mimpid=N" if > > > > needed when > > > > using the generic (rv64,rv32) CPUs. Vendor CPUs can't have their > > > > machine IDs changed > > > > via command line. > > > > > > Sounds good, thanks. I did just now go and check icicle to see what it > > > would report & it does not boot. I'll go bisect... > > > > BTW how are you booting the icicle board nowadays? I remember you > > mentioning about > > some changes in the FDT being required to boot and whatnot. > > I do direct kernel boots, as the HSS doesn't work anymore, and just lie > a bit to QEMU about how much DDR we have. > .PHONY: qemu-icicle > qemu-icicle: > $(qemu) -M microchip-icicle-kit \ > -m 3G -smp 5 \ > -kernel $(vmlinux_bin) \ > -dtb $(icicle_dtb) \ > -initrd $(initramfs) \ > -display none -serial null \ > -serial stdio \ > -D qemu.log -d unimp > > The platform only supports 2 GiB of DDR, not 3, but if I pass 2 to QEMU > it thinks there's 1 GiB at 0x8000_ and 1 GiB at 0x10__. The > upstream devicetree (and current FPGA reference design) expects there to > be 1 GiB at 0x8000_ and 1 GiB at 0x10_4000_. If I lie to QEMU, > it thinks there is 1 GiB at 0x8000_ and 2 GiB at 0x10__, and > things just work. I prefer doing it this way than having to modify the > DT, it is a lot easier to explain to people this way. > > I've been meaning to work the support for the icicle & mpfs in QEMU, but > it just gets shunted down the priority list. I'd really like if a proper > boot flow would run in QEMU, which means fixing whatever broke the HSS, > but I've recently picked up maintainership of dt-binding stuff in Linux, > so I've unfortunately got even less time to try and work on it. Maybe > we'll get some new graduate in and I can make them suffer in my stead... > > > If it's not too hard I'll add it in my test scripts to keep it under check. > > Perhaps > > we can even add it to QEMU testsuite. > > I don't think it really should be that bad, at least for the direct > kernel boot, which is what I mainly care about, since I use it fairly > often for debugging boot stuff in Linux. > > Anyways, aa903cf31391dd505b399627158f1292a6d19896 is the first bad commit: > commit aa903cf31391dd505b399627158f1292a6d19896 > Author: Bin Meng > Date: Fri Jun 30 23:36:04 2023 +0800 > > roms/opensbi: Upgrade from v1.2 to v1.3 > > Upgrade OpenSBI from v1.2 to v1.3 and the pre-built bios images. > > And I see something like: > qemu//build/qemu-system-riscv64 -M microchip-icicle-kit \ > -m 3G -smp 5 \ > -kernel vmlinux.bin \ > -dtb icicle.dtb \ > -initrd initramfs.cpio.gz \ > -display none -serial null \ > -serial stdio \ > -D qemu.log -d unimp > qemu-system-riscv64: warning: disabling zca extension for hart > 0x because privilege spec version does not match > qemu-system-riscv64: warning: disabling zca extension for hart > 0x0001 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zcd extension for hart > 0x0001 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zca extension for hart > 0x0002 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zcd extension for hart > 0x0002 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zca extension for hart > 0x0003 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zcd extension for hart > 0x0003 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zca extension for hart > 0x00
Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type
On Wed, Jul 12, 2023 at 06:39:28PM -0300, Daniel Henrique Barboza wrote: > On 7/12/23 18:35, Conor Dooley wrote: > > On Wed, Jul 12, 2023 at 06:09:10PM -0300, Daniel Henrique Barboza wrote: > > > > > It is intentional. Those default marchid/mimpid vals were derived from > > > the current > > > QEMU version ID/build and didn't mean much. > > > > > > It is still possible to set them via "-cpu rv64,marchid=N,mimpid=N" if > > > needed when > > > using the generic (rv64,rv32) CPUs. Vendor CPUs can't have their machine > > > IDs changed > > > via command line. > > > > Sounds good, thanks. I did just now go and check icicle to see what it > > would report & it does not boot. I'll go bisect... > > BTW how are you booting the icicle board nowadays? I remember you mentioning > about > some changes in the FDT being required to boot and whatnot. I do direct kernel boots, as the HSS doesn't work anymore, and just lie a bit to QEMU about how much DDR we have. .PHONY: qemu-icicle qemu-icicle: $(qemu) -M microchip-icicle-kit \ -m 3G -smp 5 \ -kernel $(vmlinux_bin) \ -dtb $(icicle_dtb) \ -initrd $(initramfs) \ -display none -serial null \ -serial stdio \ -D qemu.log -d unimp The platform only supports 2 GiB of DDR, not 3, but if I pass 2 to QEMU it thinks there's 1 GiB at 0x8000_ and 1 GiB at 0x10__. The upstream devicetree (and current FPGA reference design) expects there to be 1 GiB at 0x8000_ and 1 GiB at 0x10_4000_. If I lie to QEMU, it thinks there is 1 GiB at 0x8000_ and 2 GiB at 0x10__, and things just work. I prefer doing it this way than having to modify the DT, it is a lot easier to explain to people this way. I've been meaning to work the support for the icicle & mpfs in QEMU, but it just gets shunted down the priority list. I'd really like if a proper boot flow would run in QEMU, which means fixing whatever broke the HSS, but I've recently picked up maintainership of dt-binding stuff in Linux, so I've unfortunately got even less time to try and work on it. Maybe we'll get some new graduate in and I can make them suffer in my stead... > If it's not too hard I'll add it in my test scripts to keep it under check. > Perhaps > we can even add it to QEMU testsuite. I don't think it really should be that bad, at least for the direct kernel boot, which is what I mainly care about, since I use it fairly often for debugging boot stuff in Linux. Anyways, aa903cf31391dd505b399627158f1292a6d19896 is the first bad commit: commit aa903cf31391dd505b399627158f1292a6d19896 Author: Bin Meng Date: Fri Jun 30 23:36:04 2023 +0800 roms/opensbi: Upgrade from v1.2 to v1.3 Upgrade OpenSBI from v1.2 to v1.3 and the pre-built bios images. And I see something like: qemu//build/qemu-system-riscv64 -M microchip-icicle-kit \ -m 3G -smp 5 \ -kernel vmlinux.bin \ -dtb icicle.dtb \ -initrd initramfs.cpio.gz \ -display none -serial null \ -serial stdio \ -D qemu.log -d unimp qemu-system-riscv64: warning: disabling zca extension for hart 0x because privilege spec version does not match qemu-system-riscv64: warning: disabling zca extension for hart 0x0001 because privilege spec version does not match qemu-system-riscv64: warning: disabling zcd extension for hart 0x0001 because privilege spec version does not match qemu-system-riscv64: warning: disabling zca extension for hart 0x0002 because privilege spec version does not match qemu-system-riscv64: warning: disabling zcd extension for hart 0x0002 because privilege spec version does not match qemu-system-riscv64: warning: disabling zca extension for hart 0x0003 because privilege spec version does not match qemu-system-riscv64: warning: disabling zcd extension for hart 0x0003 because privilege spec version does not match qemu-system-riscv64: warning: disabling zca extension for hart 0x0004 because privilege spec version does not match qemu-system-riscv64: warning: disabling zcd extension for hart 0x0004 because privilege spec version does not match OpenSBI v1.3 _ _ / __ \ / | _ \_ _| | | | |_ __ ___ _ __ | (___ | |_) || | | | | | '_ \ / _ \ '_ \ \___ \| _ < | | | |__| | |_) | __/ | | |) | |_) || |_ \/| .__/ \___|_| |_|_/|___/_| | | |_| init_coldboot: ipi init failed (error -1009) Just to note, because we use our own firmware that vendors in OpenSBI and compiles only a significantly cut down number of files from it, we do not use the fw_dynamic etc flow on our hardware. As a re
Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type
On Wed, Jul 12, 2023 at 06:09:10PM -0300, Daniel Henrique Barboza wrote: > It is intentional. Those default marchid/mimpid vals were derived from the > current > QEMU version ID/build and didn't mean much. > > It is still possible to set them via "-cpu rv64,marchid=N,mimpid=N" if needed > when > using the generic (rv64,rv32) CPUs. Vendor CPUs can't have their machine IDs > changed > via command line. Sounds good, thanks. I did just now go and check icicle to see what it would report & it does not boot. I'll go bisect... signature.asc Description: PGP signature
Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type
On Wed, Jul 12, 2023 at 05:30:41PM -0300, Daniel Henrique Barboza wrote: > On 7/12/23 16:22, Conor Dooley wrote: > > On Wed, Jul 12, 2023 at 04:01:48PM -0300, Daniel Henrique Barboza wrote: > > > The 'max' CPU type is used by tooling to determine what's the most > > > capable CPU a current QEMU version implements. Other archs such as ARM > > > implements this type. Let's add it to RISC-V. > > > > > > What we consider "most capable CPU" in this context are related to > > > ratified, non-vendor extensions. This means that we want the 'max' CPU > > > to enable all (possible) ratified extensions by default. The reasoning > > > behind this design is (1) vendor extensions can conflict with each other > > > and we won't play favorities deciding which one is default or not and > > > (2) non-ratified extensions are always prone to changes, not being > > > stable enough to be enabled by default. > > > > > > All this said, we're still not able to enable all ratified extensions > > > due to conflicts between them. Zfinx and all its dependencies aren't > > > enabled because of a conflict with RVF. zce, zcmp and zcmt are also > > > disabled due to RVD conflicts. When running with 64 bits we're also > > > disabling zcf. > > > > > > Signed-off-by: Daniel Henrique Barboza > > > > This seems like it will be super helpful for CI stuff etc, thanks for > > doing it. > > And Linux actually boots on it, which was remarkable to see. I was expecting > something > to blow up I guess. > > This is the riscv,isa DT generated: > > # cat /proc/device-tree/cpus/cpu@0/riscv,isa > rv64imafdch_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_zfh_zfhmin_zca_zcb_zcd_ > zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_zkne_zknh_zkr_zks_zksed_zksh_zkt_ > zve32f_zve64f_zve64d_smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt# Of which an upstream Linux kernel, building using something close to defconfig, accepts only rv64imafdch_zicbom_zicboz_zicntr_zicsr_zifencei_zihintpause_zihpm_zba_zbb_zbs_sscofpmf_sstc_svinval_svnapot_svpbmt so the set of possible things that break could break it has been reduced somewhat. btw, I noticed that the default marchid/mimpid have changed. Previously I used to see something like: processor : 15 hart: 15 isa : rv64imafdcvh_zicbom_zicboz_zicntr_zicsr_zifencei_zihintpause_zihpm_zba_zbb_zbs_sscofpmf_sstc mmu : sv57 mvendorid : 0x0 marchid : 0x80032 mimpid : 0x80032 in /proc/cpuinfo, but "now" I see 0x0 for marchid & mimpid. Is this change to the default behaviour intentional? I saw "now" in "s because I applied your patches on top of Alistair's next branch, which contains the changes to m*id stuff. Cheers, Conor. signature.asc Description: PGP signature
Re: [PATCH for-8.2 6/7] target/riscv: add 'max' CPU type
On Wed, Jul 12, 2023 at 04:01:48PM -0300, Daniel Henrique Barboza wrote: > The 'max' CPU type is used by tooling to determine what's the most > capable CPU a current QEMU version implements. Other archs such as ARM > implements this type. Let's add it to RISC-V. > > What we consider "most capable CPU" in this context are related to > ratified, non-vendor extensions. This means that we want the 'max' CPU > to enable all (possible) ratified extensions by default. The reasoning > behind this design is (1) vendor extensions can conflict with each other > and we won't play favorities deciding which one is default or not and > (2) non-ratified extensions are always prone to changes, not being > stable enough to be enabled by default. > > All this said, we're still not able to enable all ratified extensions > due to conflicts between them. Zfinx and all its dependencies aren't > enabled because of a conflict with RVF. zce, zcmp and zcmt are also > disabled due to RVD conflicts. When running with 64 bits we're also > disabling zcf. > > Signed-off-by: Daniel Henrique Barboza This seems like it will be super helpful for CI stuff etc, thanks for doing it. signature.asc Description: PGP signature
Re: [PATCH v8 02/20] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set
On Wed, Jul 05, 2023 at 07:00:52PM -0300, Daniel Henrique Barboza wrote: > > > On 7/5/23 18:49, Conor Dooley wrote: > > On Wed, Jul 05, 2023 at 06:39:37PM -0300, Daniel Henrique Barboza wrote: > > > The absence of a satp mode in riscv_host_cpu_init() is causing the > > > following error: > > > > > > $ ./qemu/build/qemu-system-riscv64 -machine virt,accel=kvm \ > > > -m 2G -smp 1 -nographic -snapshot \ > > > -kernel ./guest_imgs/Image \ > > > -initrd ./guest_imgs/rootfs_kvm_riscv64.img \ > > > -append "earlycon=sbi root=/dev/ram rw" \ > > > -cpu host > > > ** > > > ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be > > > reached > > > Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should > > > not be reached > > > Aborted > > > > > > The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c. > > > It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map. > > > > > > For this KVM cpu we would need to inherit the satp supported modes > > > from the RISC-V host. At this moment this is not possible because the > > > KVM driver does not support it. And even when it does we can't just let > > > this broken for every other older kernel. > > > > > > Since mmu-type is not a required node, according to [1], skip the > > > 'mmu-type' FDT node if there's no satp_mode set. We'll revisit this > > > logic when we can get satp information from KVM. > > > > > > [1] > > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml > > > > I don't think this is the correct link to reference as backup, as the > > generic binding sets out no requirements. I think you would want to link > > to the RISC-V specific cpus binding. > > You mean this link? > > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/riscv/cpus.yaml Yeah, that's the correct file. Should probably have linked it, sorry about that. And in case it was not clear, not suggesting that this would require a resend, since the reasoning is correct. > > That said, things like FreeBSD and U-Boot appear to require mmu-type > > https://lore.kernel.org/all/20230705-fondue-bagginess-66c25f1a4135@spud/ > > so I am wondering if we should in fact make the mmu-type a required > > property in the RISC-V specific binding. > > > To make it required, as far as QEMU is concerned, we'll need to assume a > default value for the 'host' CPU type (e.g. sv57). In the future we can read > the > satp host value directly when/if KVM provides satp_mode via get_one_reg(). I dunno if assuming is the right thing to do, since it could be actively wrong. Leaving it out, as you are doing here, is, IMO, nicer to those guests. Once there's an API for it, I think it could then be added and then the additional guests would be supported. Thanks, Conor. signature.asc Description: PGP signature
Re: [PATCH v8 02/20] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set
On Wed, Jul 05, 2023 at 06:39:37PM -0300, Daniel Henrique Barboza wrote: > The absence of a satp mode in riscv_host_cpu_init() is causing the > following error: > > $ ./qemu/build/qemu-system-riscv64 -machine virt,accel=kvm \ > -m 2G -smp 1 -nographic -snapshot \ > -kernel ./guest_imgs/Image \ > -initrd ./guest_imgs/rootfs_kvm_riscv64.img \ > -append "earlycon=sbi root=/dev/ram rw" \ > -cpu host > ** > ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be > reached > Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should > not be reached > Aborted > > The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c. > It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map. > > For this KVM cpu we would need to inherit the satp supported modes > from the RISC-V host. At this moment this is not possible because the > KVM driver does not support it. And even when it does we can't just let > this broken for every other older kernel. > > Since mmu-type is not a required node, according to [1], skip the > 'mmu-type' FDT node if there's no satp_mode set. We'll revisit this > logic when we can get satp information from KVM. > > [1] > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml I don't think this is the correct link to reference as backup, as the generic binding sets out no requirements. I think you would want to link to the RISC-V specific cpus binding. That said, things like FreeBSD and U-Boot appear to require mmu-type https://lore.kernel.org/all/20230705-fondue-bagginess-66c25f1a4135@spud/ so I am wondering if we should in fact make the mmu-type a required property in the RISC-V specific binding. Since nommu is covered by an mmu type of "riscv,none", I am kinda struggling to think of a case where it should be left out (while describing real hardware at least). Cheers, Conor. signature.asc Description: PGP signature
Re: [PATCH] target/riscv: Fix PMU node property for virt machine
On Thu, Apr 27, 2023 at 01:28:18PM +0800, Yu-Chien Peter Lin wrote:
> Hi Conor,
>
> Thank you for your prompt response.
>
> On Fri, Apr 21, 2023 at 06:59:40PM +0100, Conor Dooley wrote:
> > On Fri, Apr 21, 2023 at 09:14:37PM +0800, Yu Chien Peter Lin wrote:
> > > The length of fdt_event_ctr_map[20] will add 5 dummy cells in
> > > "riscv,event-to-mhpmcounters" property, so directly initialize
> > > the array without an explicit size.
> > >
> > > This patch also fixes the typo of PMU cache operation result ID
> > > of MISS (0x1) in the comments, and renames event idx 0x10021 to
> > > RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.
> > >
> > > Signed-off-by: Yu Chien Peter Lin
> > > ---
> > >
> > > $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu
> > > rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
> > > [...]
> > > riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9
> > >0x02 0x02 0x7fffc
> > >0x10019 0x10019 0x7fff8
> > >0x1001b 0x1001b 0x7fff8
> > >0x10021 0x10021 0x7fff8
> > >dummy cells --->0x00 0x00 0x00 0x00 0x00>;
> > >
> > > This won't break the OpenSBI, but will cause it to incorrectly increment
> > > num_hw_events [1] to 6, and DT validation failure in kernel [2].
> > >
> > > $ dt-validate -p
> > > Documentation/devicetree/bindings/processed-schema.json virt.dtb
> > > [...]
> > > virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281],
> > > [2, 2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569,
> > > 65569, 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should
> > > not be valid under {'type': 'object'}
> >
> > I would note that this warning here does not go away with this patch ^^
> > It's still on my todo list, unless you want to fix it!
>
> I don't fully understand the warning raised by simple-bus.yaml
> is it reasonable to move pmu out of soc node?
If it has no reg properties, it should not be on the soc bus.
I previously made similar changes to other nodes, see commit
ae29379998 ("hw/riscv: virt: fix syscon subnode paths"), as I think it
is indeed the same change here.
Cheers,
Conor.
signature.asc
Description: PGP signature
Re: [PATCH] target/riscv: Fix PMU node property for virt machine
On Fri, Apr 21, 2023 at 09:14:37PM +0800, Yu Chien Peter Lin wrote:
> The length of fdt_event_ctr_map[20] will add 5 dummy cells in
> "riscv,event-to-mhpmcounters" property, so directly initialize
> the array without an explicit size.
>
> This patch also fixes the typo of PMU cache operation result ID
> of MISS (0x1) in the comments, and renames event idx 0x10021 to
> RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.
>
> Signed-off-by: Yu Chien Peter Lin
> ---
>
> $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu
> rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
> [...]
> riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9
>0x02 0x02 0x7fffc
>0x10019 0x10019 0x7fff8
>0x1001b 0x1001b 0x7fff8
>0x10021 0x10021 0x7fff8
>dummy cells --->0x00 0x00 0x00 0x00 0x00>;
>
> This won't break the OpenSBI, but will cause it to incorrectly increment
> num_hw_events [1] to 6, and DT validation failure in kernel [2].
>
> $ dt-validate -p Documentation/devicetree/bindings/processed-schema.json
> virt.dtb
> [...]
> virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2,
> 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569,
> 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should not be valid
> under {'type': 'object'}
I would note that this warning here does not go away with this patch ^^
It's still on my todo list, unless you want to fix it!
> From schema:
> /home/peterlin/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
> virt.dtb: pmu: riscv,event-to-mhpmcounters:6: [0, 0] is too short
> [...]
And with the binding below there is a 3rd warning, but I think it is
incorrect and I submitted a patch for the binding to fix it.
That said, this doesn't seem to have been submitted on top of my naive
s/20/15/ patch that Alistair picked up. Is this intended to replace, or
for another branch? Replace works fine for me, don't have sentimental
feelings for that patch!
I think your approach here was better than my s/20/15/, but seems like a
bit of a fix and a bit of cleanup all-in-one.
Either way, warnings gone so WFM :) :)
Thanks,
Conor.
> [1]
> https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_pmu.c#L255
> [2]
> https://github.com/torvalds/linux/blob/v6.3-rc7/Documentation/devicetree/bindings/perf/riscv%2Cpmu.yaml#L54-L66
> ---
> target/riscv/cpu.h| 2 +-
> target/riscv/cpu_helper.c | 2 +-
> target/riscv/pmu.c| 88 +++
> 3 files changed, 45 insertions(+), 47 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..eab518542c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -812,7 +812,7 @@ enum riscv_pmu_event_idx {
> RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> -RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> +RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS = 0x10021,
> };
>
> /* CSR function table */
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..5d3e032ec9 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1210,7 +1210,7 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu,
> MMUAccessType access_type)
>
> switch (access_type) {
> case MMU_INST_FETCH:
> -pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> +pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS;
> break;
> case MMU_DATA_LOAD:
> pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index b8e56d2b7b..0b21c3fa38 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -35,51 +35,49 @@
> */
> void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> {
> -uint32_t fdt_event_ctr_map[20] = {};
> -uint32_t cmask;
> -
> /* All the programmable counters can map to any event */
> -cmask = MAKE_32BIT_MASK(3, num_ctrs);
> -
> - /*
> -* The event encoding is specified in the SBI specification
> -* Event idx is a 20bits wide number encoded as follows:
> -* event_idx[19:16] = type
> -* event_idx[15:0] = code
> -* The code field in cache events are encoded as follows:
> -* event_idx.code[15:3] = cache_id
> -* event_idx.code[2:1] = op_id
> -* event_idx.code[0:0] = result_id
> -*/
> -
> - /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> - fdt_event_ctr_map[0] = cpu_to_be32(0x0001);
> - fdt_event_ctr_map[1] = cpu_to_be32(0x0001);
> - fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
> -
> - /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> - fdt_event_ctr_map[3]
[PATCH] target/riscv: fix invalid riscv,event-to-mhpmcounters entry
From: Conor Dooley
dt-validate complains:
> soc: pmu: {'riscv,event-to-mhpmcounters':
> [[1, 1, 524281], [2, 2, 524284], [65561, 65561, 524280],
> [65563, 65563, 524280], [65569, 65569, 524280], [0, 0, 0], [0, 0]],
> pmu: riscv,event-to-mhpmcounters:6: [0, 0] is too short
There are bogus 0 entries added at the end, of which one is of
insufficient length. This happens because only 15 of
fdt_event_ctr_map[]'s 20 elements are populated & qemu_fdt_setprop() is
called using the size of the array.
Reduce the array to 15 elements to make the error go away.
Signed-off-by: Conor Dooley
---
I dunno if I am missing something intentional here, feel free to scream
if so!
---
target/riscv/pmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index b8e56d2b7b..fa1e1484c2 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -35,7 +35,7 @@
*/
void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
{
-uint32_t fdt_event_ctr_map[20] = {};
+uint32_t fdt_event_ctr_map[15] = {};
uint32_t cmask;
/* All the programmable counters can map to any event */
--
2.39.2
Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
On Thu, Feb 02, 2023 at 03:37:17PM -0300, Daniel Henrique Barboza wrote: > On 2/2/23 14:25, Conor Dooley wrote: > > On Thu, Feb 02, 2023 at 10:58:07AM -0300, Daniel Henrique Barboza wrote: > > > This new version removed the translate_fn() from patch 1 because it > > > wasn't removing the sign-extension for pentry as we thought it would. > > > A more detailed explanation is given in the commit msg of patch 1. > > > > > > We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and > > > using it when we're running a 32-bit CPU. This worked with 32 bit > > > 'virt' machine booting with the -kernel option. > > > > > > If this approach doesn't work for the Xvisor use case, IMO we should > > > just filter kernel_load_addr bits directly as we were doing a handful of > > > versions ago. > > > > > > Patches are based on current riscv-to-apply.next. > > > > > > Changes from v9: > > > - patch 1: > > >- removed the translate_fn() callback > > >- return 'kernel_low' when running a 32-bit CPU > > > - v9 link: > > > https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html > > > > I think my T-b got lost last time around, but I gave this version a > > whirl too & things are working for me as they were before on Icicle. > > That was my bad. I forgot to add the test-by after doing the changes for > the next version. Oh, I'm sorry. I saw a new version of the series a few days ago and noticed the missing tags, and then saw this one today, touching MPFS, and conflated the two. > But I don't think this is the series you're talking about. The tested-by tag > you gave was on these patches: > > "[PATCH v5 0/3] riscv_load_fdt() semantics change" > > I believe you can add a Tested-by there. And feel free to give it a go - the > patches are on riscv-to-apply.next already. Tested-by stands here though, I replied to the same message-id that I shazamed and tried ;) And I did so on top of the HEAD of riscv-to-apply.next, so I am happy with the version that got applied too. Sorry! signature.asc Description: PGP signature
Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
On Thu, Feb 02, 2023 at 10:58:07AM -0300, Daniel Henrique Barboza wrote: > Hi, > > This new version removed the translate_fn() from patch 1 because it > wasn't removing the sign-extension for pentry as we thought it would. > A more detailed explanation is given in the commit msg of patch 1. > > We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and > using it when we're running a 32-bit CPU. This worked with 32 bit > 'virt' machine booting with the -kernel option. > > If this approach doesn't work for the Xvisor use case, IMO we should > just filter kernel_load_addr bits directly as we were doing a handful of > versions ago. > > Patches are based on current riscv-to-apply.next. > > Changes from v9: > - patch 1: > - removed the translate_fn() callback > - return 'kernel_low' when running a 32-bit CPU > - v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html I think my T-b got lost last time around, but I gave this version a whirl too & things are working for me as they were before on Icicle. For the series: Tested-by: Conor Dooley Thanks, Conor. > > Daniel Henrique Barboza (3): > hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel() > hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel() > hw/riscv/boot.c: make riscv_load_initrd() static > > hw/riscv/boot.c| 96 +++--- > hw/riscv/microchip_pfsoc.c | 12 + > hw/riscv/opentitan.c | 4 +- > hw/riscv/sifive_e.c| 4 +- > hw/riscv/sifive_u.c| 12 + > hw/riscv/spike.c | 14 ++ > hw/riscv/virt.c| 12 + > include/hw/riscv/boot.h| 3 +- > 8 files changed, 76 insertions(+), 81 deletions(-) > > -- > 2.39.1 > > signature.asc Description: PGP signature
Re: [PATCH v4 0/3] riscv_load_fdt() semantics change
On Thu, Jan 26, 2023 at 10:52:16AM -0300, Daniel Henrique Barboza wrote: > Hi, > > After discussions in the previous version, where we ended up discovering > the details of why the current riscv_load_fdt() works with the Microchip > Icicle Kit board almost by accident, I decided to change how > riscv_compute_fdt_addr() (the FDT address calculation from > riscv_load_fdt()) operates. > > Instead of relying on premises that the Icicle Kit board can't hold > right from start, since dram_base + mem_size will never be contained in > a contiguous RAM area, change the FDT address calculation to also > receive the bondaries of the DRAM block that the board guarantees that > it's not sparse. With this extra information we're able to make a more > consistent FDT address calculation that will cover all existing cases we > have today. The "test" case that fail before, is now back passing again. Thanks Daniel! Tested-by: Conor Dooley signature.asc Description: PGP signature
Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt address function
On Sat, Jan 21, 2023 at 02:58:19PM -0300, Daniel Henrique Barboza wrote:
> Conor,
>
> Thanks for the Icicle-kit walk-through!
nw chief
> I'll not claim that I fully understood it,
> but I understood enough to handle the situation ATM.
tbf, I struggle to explain/visualise that stuff with the "windows" etc
well. I wrote myself a program to visualise it for a good reason!
Well it was done in Rust, so there were two good reasons ;)
> Without this change, this is where the FDT is being installed in the board
> when
> I start it with 8Gb of RAM (retrieved via 'info roms'):
>
> addr=bfe0 size=0x00a720 mem=ram name="fdt"
>
> Which surprised me at first because this is almost at the end of the LO area
> which has
> 1Gb and I figured it would be in the middle of another RAM area. I took
> another read
> at what we're doing in riscv_load_fdt():
>
> ---
> temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> ---
>
> This code can be read as "if the starting address of the RAM is lower than
> 3Gb, put
> the FDT no further than 3Gb (0xc000). Otherwise, put it at the end of
> dram",
> where "dram_base" is the starting address of the RAM block that the function
> receives.
>
> For icicle-kit, this is being passed as memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> 0x8000, which is 2Gb.
>
> So, regardless of how much RAM we have (dram_end), the FDT will always be
> capped at
> 3Gb. At this moment, this fits exactly at the end of the LO area for the
> Icicle Kit.
> Which is funny because this 3Gb restriction was added by commit 1a475d39ef54
> to fix
> 32 bit guest boot and it happened to also work for the Microchip SoC.
That's hilariously convenient hahah
> So yeah, I thought that I was fixing a bug and in the end I caused one. This
> patch
> needs to go.
>
> Alistair, I believe I should re-send v2, this time explaining why the
> existing function
> will not break the Microchip board because we'll never put the FDT out of the
> LO area
> of the board. Does this work for you?
> Conor, one more thing:
>
>
> On 1/19/23 21:15, Conor Dooley wrote:
> > Hey Daniel,
> >
> > Got through the stuff I wanted to get done tonight faster than
> > expected...
> >
> > On Thu, Jan 19, 2023 at 05:17:33PM -0300, Daniel Henrique Barboza wrote:
> > > Are you testing it by using the command line
> > > you mentioned in the "qemu icicle kit es" thread?
> > >
> > > $(QEMU)/qemu-system-riscv64 \
> > > -M microchip-icicle-kit \
> > > -m 2G -smp 5 \
> > > -kernel $(vmlinux_bin) \
> > > -dtb $(devkit).dtb \
> > > -initrd $(initramfs) \
> > > -display none \
> > > -serial null \
> > > -serial stdio
> >
> > Yah, effectively. It's not quite that, but near enough as makes no real
> > difference:
> > qemu-icicle:
> > $(QEMU)/qemu-system-riscv64 -M microchip-icicle-kit \
> > -m 2G -smp 5 \
> > -kernel $(vmlinux_bin) \
> > -dtb $(wrkdir)/riscvpc.dtb \
> > -initrd $(initramfs) \
> > -display none -serial null \
> > -serial stdio \
> > -D qemu.log -d unimp
> >
> > I just tried to make things somewhat more intelligible for that thread.
>
> I tried it out with kernel v6.0.0 (I saw you mentioning in the other thread
> that
> this was the latest kernel you were able to boot this way)
Yah, I said that because I didn't want them to have to mess with DT.
Later kernels do work, but need DT modifications as things are now
configured for the below case.
> > The current Devicetree in Linux & U-Boot corresponds to a configuration
> > like:
> > ⢰⡖⠒⠒⠒⣶⠒0x8000
> > ⢸⡇ ⣿ ⡇
> > ⢸⡇ ⡟ ⡇
> > ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⡇ ⡇
> > ⢸⡖⠒⠒⢲⡇ ⡇ 0x4000
> > ⢸⡇ ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⢸⡇ ⡇ ⡇
> > ⢸⡇ ⢸⡇ ⡇ ⡇ <-- 32- & 64-bit start at 0x0
> > ⠘⠓⠒0⠚⠓⠒1⠒⠓⠒0x
> >
> > That DT looks like:
> > ddrc_cache_lo: memory@8000 {
> > device_type = "memory";
> > reg = <0x0 0x8000 0x0 0x4000>;
> > status = "okay";
> > };
> >
> > ddrc_cache_hi: memory@104000 {
> > device_type = &quo
Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt address function
Hey Daniel,
Got through the stuff I wanted to get done tonight faster than
expected...
On Thu, Jan 19, 2023 at 05:17:33PM -0300, Daniel Henrique Barboza wrote:
> Are you testing it by using the command line
> you mentioned in the "qemu icicle kit es" thread?
>
> $(QEMU)/qemu-system-riscv64 \
> -M microchip-icicle-kit \
> -m 2G -smp 5 \
> -kernel $(vmlinux_bin) \
> -dtb $(devkit).dtb \
> -initrd $(initramfs) \
> -display none \
> -serial null \
> -serial stdio
Yah, effectively. It's not quite that, but near enough as makes no real
difference:
qemu-icicle:
$(QEMU)/qemu-system-riscv64 -M microchip-icicle-kit \
-m 2G -smp 5 \
-kernel $(vmlinux_bin) \
-dtb $(wrkdir)/riscvpc.dtb \
-initrd $(initramfs) \
-display none -serial null \
-serial stdio \
-D qemu.log -d unimp
I just tried to make things somewhat more intelligible for that thread.
Also in case it is not obvious, I do work for Microchip. As I mentioned
to Alistair at LPC, I/we don't have the cycles at the moment to do
anything with QEMU, so the bits of fixes I have sent are things I fixed
while debugging other issues etc, mostly in the evenings.
Anways, I'll attempt to explain what the craic is here..
On Thu, Jan 19, 2023 at 04:17:24PM -0300, Daniel Henrique Barboza wrote:
> The Icicle Kit board works with 2 distinct RAM banks that are separated
Ehh, 2 isn't really true. There are 6 possible "windows" into the DDR on
MPFS, list here as with their start addresses.
32-bit cached 0x008000
64-bit cached 0x10
32-bit non-cached 0x00c000
64-bit non-cached 0x14
32-bit WCB0x00d000
64-bit WCB0x18
These are the "bus" addresses, where the harts think the memory is, but
the memory is not actually connected there. There are some runtime
configurable registers which determine what addresses these correspond
to in the DDR itself.
When the QEMU port for MPFS was written, only two of these were in use,
the 32-bit and 64-bit non-cached regions. The config (seg) registers
were set up so that the 32-bit cached region pointed to 0x0 in DDR and
the 64-bit region pointed to 0x3000_ in DDR.
⢰⡖⠒⠒⠒⣶⠒0x8000
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡖⠒⠒⢲⡇ ⡇ 0x4000
⢸⡇ ⢸⡇ ⡇ ⡇
⢸⡇ ⢸⠓⠒⠒⠒⠃ ⡇ <-- 64-bit starts here
⢸⡇ ⢸ ⡇
⢸⡇ ⢸ ⡇
⢸⡇ ⢸ ⡇
⢸⡇ ⢸ ⡇
⢸⡇ ⢸ ⡇ <-- 32-bit starts at 0x0
⠘⠓⠒0⠚⠒⠒1⠒⠒⠒0x
(These diagrams are a bit crap, I'm copy pasting them from a TUI tool
for visualising these I made for myself. The ~s can be ignored.
https://github.com/ConchuOD/memory-aperature-configurator)
> by a gap. We have a lower bank with 1GiB size, a gap follows,
> then at 64GiB the high memory starts.
As you correctly pointed out, that lower region is in fact 1 GiB & hence
there is actually an overlapping region of 256 MiB.
The Devicetree at this point in time looked like:
ddrc_cache_lo: memory@8000 {
device_type = "memory";
reg = <0x0 0x8000 0x0 0x3000>;
clocks = < CLK_DDRC>;
status = "okay";
};
ddrc_cache_hi: memory@10 {
device_type = "memory";
reg = <0x10 0x0 0x0 0x4000>;
clocks = < CLK_DDRC>;
status = "okay";
};
At some point, it was decided that instead we would use a configuration
with ~no memory at 32-bit addresses. I think it was this one here:
⢰⡖⠒⠒⢲⡖⠒⠒⠒⣶⠒0x8000
⢸⡇ ⢸⡇ ⣿ ⡇
⢸⠓⠒⠒⠚⡇ ⡟ ⡇ <-- 32-bit starts here
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ 0x4000
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇ <-- 64-bit starts at 0x0
⠘⠒⠒0⠒⠓⠒1⠒⠓⠒0x
Because of how these windows work, the 32-bit cached region was always
there, just not used as the Devicetree became:
ddrc_cache: memory@10 {
device_type = "memory";
reg = <0x10 0x0 0x0 0x7600>;
status = "okay";
};
The remaining bit of memory is being used for some WCB buffers etc &
not for the OS itself. This was never upstreamed anywhere AFAIK as it
was a workaround.
The current Devicetree in Linux & U-Boot corresponds to a configuration
like:
⢰⡖⠒⠒⠒⣶⠒0x8000
⢸⡇ ⣿ ⡇
⢸⡇ ⡟ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡇ ⡇ ⡇
⢸⡖⠒⠒⢲⡇ ⡇ 0x4000
⢸⡇ ⢸⡇ ⡇ ⡇
⢸⡇ ⢸⡇ ⡇ ⡇
⢸⡇ ⢸⡇ ⡇ ⡇
⢸⡇ ⢸⡇ ⡇ ⡇
⢸⡇ ⢸⡇ ⡇ ⡇
⢸⡇ ⢸⡇
Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt address function
Hey! On Thu, Jan 19, 2023 at 04:17:24PM -0300, Daniel Henrique Barboza wrote: > The Icicle Kit board works with 2 distinct RAM banks that are separated > by a gap. We have a lower bank with 1GiB size, a gap follows, > then at 64GiB the high memory starts. > > MachineClass::default_ram_size is set to 1.5Gb and machine_init() is > enforcing it as minimal RAM size, meaning that there we'll always have > at least 512 MiB in the Hi RAM area, and that the FDT will be located > there all the time. > > riscv_compute_fdt_addr() can't handle this setup because it assumes that > the RAM is always contiguous. It's also returning an uint32_t because > it's enforcing that fdt address is sitting on an area that is addressable > to 32 bit CPUs, but 32 bits won't be enough to point to the Hi area of > the Icicle Kit RAM (and to its FDT itself). > > Create a new function called microchip_compute_fdt_addr() that is able > to deal with all these details that are particular to the Icicle Kit. > Ditch riscv_compute_fdt_addr() and use it instead. Hmm, this breaks boot for me in what is a valid configuration for Icicle/PolarFire SoC which was previously functional in QEMU. I'll try and write another email explaining things in more detail, but in case I do not have time to get that done in the next day or two I figured I should let you know. Thanks, Conor. signature.asc Description: PGP signature
Re: [PATCH 09/15] hw/riscv: microchip_pfsoc: Fix the number of interrupt sources of PLIC
On Thu, Dec 01, 2022 at 10:08:05PM +0800, Bin Meng wrote:
> Per chapter 6.5.2 in [1], the number of interupt sources including
> interrupt source 0 should be 187.
>
> [1] PolarFire SoC MSS TRM:
> https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/ReferenceManuals/PolarFire_SoC_FPGA_MSS_Technical_Reference_Manual_VC.pdf
Reviewed-by: Conor Dooley
Thanks!
>
> Fixes: 56f6e31e7b7e ("hw/riscv: Initial support for Microchip PolarFire SoC
> Icicle Kit board")
> Signed-off-by: Bin Meng
> ---
>
> include/hw/riscv/microchip_pfsoc.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/riscv/microchip_pfsoc.h
> b/include/hw/riscv/microchip_pfsoc.h
> index a757b240e0..9720bac2d5 100644
> --- a/include/hw/riscv/microchip_pfsoc.h
> +++ b/include/hw/riscv/microchip_pfsoc.h
> @@ -150,7 +150,7 @@ enum {
> #define MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT1
> #define MICROCHIP_PFSOC_COMPUTE_CPU_COUNT 4
>
> -#define MICROCHIP_PFSOC_PLIC_NUM_SOURCES185
> +#define MICROCHIP_PFSOC_PLIC_NUM_SOURCES187
> #define MICROCHIP_PFSOC_PLIC_NUM_PRIORITIES 7
> #define MICROCHIP_PFSOC_PLIC_PRIORITY_BASE 0x04
> #define MICROCHIP_PFSOC_PLIC_PENDING_BASE 0x1000
> --
> 2.34.1
>
>
>
signature.asc
Description: PGP signature
Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote:
> Qemu virt machine can support few cache events and cycle/instret counters.
> It also supports counter overflow for these events.
>
> Add a DT node so that OpenSBI/Linux kernel is aware of the virt machine
> capabilities. There are some dummy nodes added for testing as well.
Hey Atish!
I was fiddling with dumping the virt machine dtb again today to check
some dt-binding changes I was making for the isa string would play
nicely with the virt machine & I noticed that this patch has introduced
a new validation failure:
./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json
qemu.dtb
/home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1,
1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569,
65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be
valid under {'type': 'object'}
From schema:
/home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
I assume this is the aforementioned "dummy" node & you have no intention
of creating a binding for this?
Thanks,
Conor.
> Acked-by: Alistair Francis
> Signed-off-by: Atish Patra
> Signed-off-by: Atish Patra
> ---
> hw/riscv/virt.c| 16 +
> target/riscv/pmu.c | 57 ++
> target/riscv/pmu.h | 1 +
> 3 files changed, 74 insertions(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ff8c0df5cd47..befa9d2c26ac 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -30,6 +30,7 @@
> #include "hw/char/serial.h"
> #include "target/riscv/cpu.h"
> #include "hw/core/sysbus-fdt.h"
> +#include "target/riscv/pmu.h"
> #include "hw/riscv/riscv_hart.h"
> #include "hw/riscv/virt.h"
> #include "hw/riscv/boot.h"
> @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
> aplic_phandles[socket] = aplic_s_phandle;
> }
>
> +static void create_fdt_pmu(RISCVVirtState *s)
> +{
> +char *pmu_name;
> +MachineState *mc = MACHINE(s);
> +RISCVCPU hart = s->soc[0].harts[0];
> +
> +pmu_name = g_strdup_printf("/soc/pmu");
> +qemu_fdt_add_subnode(mc->fdt, pmu_name);
> +qemu_fdt_setprop_string(mc->fdt, pmu_name, "compatible", "riscv,pmu");
> +riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name);
> +
> +g_free(pmu_name);
> +}
> +
> static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
> bool is_32_bit, uint32_t *phandle,
> uint32_t *irq_mmio_phandle,
> @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const
> MemMapEntry *memmap,
>
> create_fdt_flash(s, memmap);
> create_fdt_fw_cfg(s, memmap);
> +create_fdt_pmu(s);
>
> update_bootargs:
> if (cmdline && *cmdline) {
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index a5f504e53c88..b8e56d2b7b8e 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -20,11 +20,68 @@
> #include "cpu.h"
> #include "pmu.h"
> #include "sysemu/cpu-timers.h"
> +#include "sysemu/device_tree.h"
>
> #define RISCV_TIMEBASE_FREQ 10 /* 1Ghz */
> #define MAKE_32BIT_MASK(shift, length) \
> (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
>
> +/*
> + * To keep it simple, any event can be mapped to any programmable counters in
> + * QEMU. The generic cycle & instruction count events can also be monitored
> + * using programmable counters. In that case, mcycle & minstret must continue
> + * to provide the correct value as well. Heterogeneous PMU per hart is not
> + * supported yet. Thus, number of counters are same across all harts.
> + */
> +void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> +{
> +uint32_t fdt_event_ctr_map[20] = {};
> +uint32_t cmask;
> +
> +/* All the programmable counters can map to any event */
> +cmask = MAKE_32BIT_MASK(3, num_ctrs);
> +
> + /*
> +* The event encoding is specified in the SBI specification
> +* Event idx is a 20bits wide number encoded as follows:
> +* event_idx[19:16] = type
> +* event_idx[15:0] = code
> +* The code field in cache events are encoded as follows:
> +* event_idx.code[15:3] = cache_id
> +* event_idx.code[2:1] = op_id
> +* event_idx.code[0:0] = result_id
> +*/
> +
> + /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> + fdt_event_ctr_map[0] = cpu_to_be32(0x0001);
> + fdt_event_ctr_map[1] = cpu_to_be32(0x0001);
> + fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
> +
> + /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> + fdt_event_ctr_map[3] = cpu_to_be32(0x0002);
> + fdt_event_ctr_map[4] = cpu_to_be32(0x0002);
> + fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
> +
> + /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01)
[PATCH v3 3/3] hw/{misc, riscv}: pfsoc: add system controller as unimplemented
From: Conor Dooley
The system controller on PolarFire SoC is access via a mailbox. The
control registers for this mailbox lie in the "IOSCB" region & the
interrupt is cleared via write to the "SYSREG" region. It also has a
QSPI controller, usually connected to a flash chip, that is used for
storing FPGA bitstreams and used for In-Application Programming (IAP).
Linux has an implementation of the system controller, through which the
hwrng is accessed, leading to load/store access faults.
Add the QSPI as unimplemented and a very basic (effectively
unimplemented) version of the system controller's mailbox. Rather than
purely marking the regions as unimplemented, service the mailbox
requests by reporting failures and raising the interrupt so a guest can
better handle the lack of support.
Signed-off-by: Conor Dooley
---
hw/misc/mchp_pfsoc_ioscb.c | 72 -
hw/misc/mchp_pfsoc_sysreg.c | 18 ++--
hw/riscv/microchip_pfsoc.c | 6 +++
include/hw/misc/mchp_pfsoc_ioscb.h | 3 ++
include/hw/misc/mchp_pfsoc_sysreg.h | 1 +
include/hw/riscv/microchip_pfsoc.h | 1 +
6 files changed, 95 insertions(+), 6 deletions(-)
diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
index f976e42f72..a71d134295 100644
--- a/hw/misc/mchp_pfsoc_ioscb.c
+++ b/hw/misc/mchp_pfsoc_ioscb.c
@@ -24,6 +24,7 @@
#include "qemu/bitops.h"
#include "qemu/log.h"
#include "qapi/error.h"
+#include "hw/irq.h"
#include "hw/sysbus.h"
#include "hw/misc/mchp_pfsoc_ioscb.h"
@@ -34,6 +35,9 @@
#define IOSCB_WHOLE_REG_SIZE0x1000
#define IOSCB_SUBMOD_REG_SIZE 0x1000
#define IOSCB_CCC_REG_SIZE 0x200
+#define IOSCB_CTRL_REG_SIZE 0x800
+#define IOSCB_QSPIXIP_REG_SIZE 0x200
+
/*
* There are many sub-modules in the IOSCB module.
@@ -45,6 +49,8 @@
#define IOSCB_LANE01_BASE 0x0650
#define IOSCB_LANE23_BASE 0x0651
#define IOSCB_CTRL_BASE 0x0702
+#define IOSCB_QSPIXIP_BASE 0x07020100
+#define IOSCB_MAILBOX_BASE 0x07020800
#define IOSCB_CFG_BASE 0x0708
#define IOSCB_CCC_BASE 0x0800
#define IOSCB_PLL_MSS_BASE 0x0E001000
@@ -143,6 +149,58 @@ static const MemoryRegionOps mchp_pfsoc_io_calib_ddr_ops =
{
.endianness = DEVICE_LITTLE_ENDIAN,
};
+#define SERVICES_CR 0x50
+#define SERVICES_SR 0x54
+#define SERVICES_STATUS_SHIFT 16
+
+static uint64_t mchp_pfsoc_ctrl_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+uint32_t val = 0;
+
+switch (offset) {
+case SERVICES_SR:
+/*
+ * Although some services have no error codes, most do. All services
+ * that do implement errors, begin their error codes at 1. Treat all
+ * service requests as failures & return 1.
+ * See the "PolarFire® FPGA and PolarFire SoC FPGA System Services"
+ * user guide for more information on service error codes.
+ */
+val = 1u << SERVICES_STATUS_SHIFT;
+break;
+default:
+qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read "
+ "(size %d, offset 0x%" HWADDR_PRIx ")\n",
+ __func__, size, offset);
+}
+
+return val;
+}
+
+static void mchp_pfsoc_ctrl_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned size)
+{
+MchpPfSoCIoscbState *s = opaque;
+
+switch (offset) {
+case SERVICES_CR:
+qemu_irq_raise(s->irq);
+break;
+default:
+qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write "
+ "(size %d, value 0x%" PRIx64
+ ", offset 0x%" HWADDR_PRIx ")\n",
+ __func__, size, value, offset);
+}
+}
+
+static const MemoryRegionOps mchp_pfsoc_ctrl_ops = {
+.read = mchp_pfsoc_ctrl_read,
+.write = mchp_pfsoc_ctrl_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
static void mchp_pfsoc_ioscb_realize(DeviceState *dev, Error **errp)
{
MchpPfSoCIoscbState *s = MCHP_PFSOC_IOSCB(dev);
@@ -162,10 +220,18 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
Error **errp)
"mchp.pfsoc.ioscb.lane23", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_LANE23_BASE, >lane23);
-memory_region_init_io(>ctrl, OBJECT(s), _pfsoc_dummy_ops, s,
- "mchp.pfsoc.ioscb.ctrl", IOSCB_SUBMOD_REG_SIZE);
+memory_region_init_io(>ctrl, OBJECT(s), _pfsoc_ctrl_ops, s,
+ "mchp.pfsoc.ioscb.ctrl", IOSCB_CTRL_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_CTRL_BASE, >ctrl);
[PATCH v3 2/3] hw/riscv: pfsoc: add missing FICs as unimplemented
From: Conor Dooley
The Fabric Interconnect Controllers provide interfaces between the FPGA
fabric and the core complex. There are 5 FICs on PolarFire SoC, numbered
0 through 4. FIC2 is an AXI4 slave interface from the FPGA fabric and
does not show up on the MSS memory map. FIC4 is dedicated to the User
Crypto Processor and does not show up on the MSS memory map either.
FIC 0, 1 & 3 do show up in the MSS memory map and neither FICs 0 or 1
are represented in QEMU, leading to load access violations while booting
Linux for Icicle if PCIe is enabled as the root port is connected via
either FIC 0 or 1.
Acked-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/riscv/microchip_pfsoc.c | 115 -
include/hw/riscv/microchip_pfsoc.h | 2 +
2 files changed, 65 insertions(+), 52 deletions(-)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index a821263d4f..2a24e3437a 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -86,58 +86,61 @@
* describes the complete IOSCB modules memory maps
*/
static const MemMapEntry microchip_pfsoc_memmap[] = {
-[MICROCHIP_PFSOC_RSVD0] = {0x0, 0x100 },
-[MICROCHIP_PFSOC_DEBUG] = { 0x100, 0xf00 },
-[MICROCHIP_PFSOC_E51_DTIM] ={ 0x100, 0x2000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT0] ={ 0x170, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT1] ={ 0x1701000, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT2] ={ 0x1702000, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT3] ={ 0x1703000, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT4] ={ 0x1704000, 0x1000 },
-[MICROCHIP_PFSOC_CLINT] = { 0x200,0x1 },
-[MICROCHIP_PFSOC_L2CC] ={ 0x201, 0x1000 },
-[MICROCHIP_PFSOC_DMA] = { 0x300, 0x10 },
-[MICROCHIP_PFSOC_L2LIM] = { 0x800, 0x200 },
-[MICROCHIP_PFSOC_PLIC] ={ 0xc00, 0x400 },
-[MICROCHIP_PFSOC_MMUART0] = { 0x2000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG0] = { 0x20001000, 0x1000 },
-[MICROCHIP_PFSOC_SYSREG] = { 0x20002000, 0x2000 },
-[MICROCHIP_PFSOC_AXISW] = { 0x20004000, 0x1000 },
-[MICROCHIP_PFSOC_MPUCFG] = { 0x20005000, 0x1000 },
-[MICROCHIP_PFSOC_FMETER] = { 0x20006000, 0x1000 },
-[MICROCHIP_PFSOC_DDR_SGMII_PHY] = { 0x20007000, 0x1000 },
-[MICROCHIP_PFSOC_EMMC_SD] = { 0x20008000, 0x1000 },
-[MICROCHIP_PFSOC_DDR_CFG] = { 0x2008,0x4 },
-[MICROCHIP_PFSOC_MMUART1] = { 0x2010, 0x1000 },
-[MICROCHIP_PFSOC_MMUART2] = { 0x20102000, 0x1000 },
-[MICROCHIP_PFSOC_MMUART3] = { 0x20104000, 0x1000 },
-[MICROCHIP_PFSOC_MMUART4] = { 0x20106000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG1] = { 0x20101000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG2] = { 0x20103000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG3] = { 0x20105000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG4] = { 0x20106000, 0x1000 },
-[MICROCHIP_PFSOC_SPI0] ={ 0x20108000, 0x1000 },
-[MICROCHIP_PFSOC_SPI1] ={ 0x20109000, 0x1000 },
-[MICROCHIP_PFSOC_I2C0] ={ 0x2010a000, 0x1000 },
-[MICROCHIP_PFSOC_I2C1] ={ 0x2010b000, 0x1000 },
-[MICROCHIP_PFSOC_CAN0] ={ 0x2010c000, 0x1000 },
-[MICROCHIP_PFSOC_CAN1] ={ 0x2010d000, 0x1000 },
-[MICROCHIP_PFSOC_GEM0] ={ 0x2011, 0x2000 },
-[MICROCHIP_PFSOC_GEM1] ={ 0x20112000, 0x2000 },
-[MICROCHIP_PFSOC_GPIO0] = { 0x2012, 0x1000 },
-[MICROCHIP_PFSOC_GPIO1] = { 0x20121000, 0x1000 },
-[MICROCHIP_PFSOC_GPIO2] = { 0x20122000, 0x1000 },
-[MICROCHIP_PFSOC_RTC] = { 0x20124000, 0x1000 },
-[MICROCHIP_PFSOC_ENVM_CFG] ={ 0x2020, 0x1000 },
-[MICROCHIP_PFSOC_ENVM_DATA] = { 0x2022,0x2 },
-[MICROCHIP_PFSOC_USB] = { 0x20201000, 0x1000 },
-[MICROCHIP_PFSOC_QSPI_XIP] ={ 0x2100, 0x100 },
-[MICROCHIP_PFSOC_IOSCB] = { 0x3000, 0x1000 },
-[MICROCHIP_PFSOC_FABRIC_FIC3] = { 0x4000, 0x2000 },
-[MICROCHIP_PFSOC_DRAM_LO] = { 0x8000, 0x4000 },
-[MICROCHIP_PFSOC_DRAM_LO_ALIAS] = { 0xc000, 0x4000 },
-[MICROCHIP_PFSOC_DRAM_HI] = { 0x10,0x0 },
-[MICROCHIP_PFSOC_DRAM_HI_ALIAS] = { 0x14,0x0 },
+[MICROCHIP_PFSOC_RSVD0] = {0x0,0x100 },
+[MICROCHIP_PFSOC_DEBUG] = { 0x100,0xf00 },
+[MICROCHIP_PFSOC_E51_DTIM] ={ 0x100, 0x2000 },
+[MICROCHIP_PFSOC_BUSERR_U
[PATCH v3 0/3] Add (more) missing PolarFire SoC io regions
From: Conor Dooley
Hey all,
Apart from DDR (see [1]), these should be the last bits needed to get
recent Linux kernels booting again for Icicle/PolarFire SoC. Previously,
I had been disabling the hwrng and PCI but I keep forgetting that is
required and decided to fix that.
I'm not entirely sure if I have done some sort of no-no thing by
registering the same interrupt with both the IOSCB and SYSREG regions.
The interrupt is raised after the system controller handles a service
via the mailbox. The mailbox's status, control and mailbox registers
are all part of the IOSCB region. It's cleared by a write to a register
in the SYSREG region.
Since my goal here is to add the regions/peripherals without actually
implementing them so that Linux etc, I'm just raising an interrupt
once a guest requests a service & reporting a status indicating that the
service request failed.
Thanks,
Conor.
1 - https://lore.kernel.org/all/Y2+dUCpd8OP52%2FDJ@spud/
Changes since v2:
- fix the actual bits in the register used for the service return
status
- remove a duplicate irq_lower() in the sysreg bits of patch 3
- move the irq raise to a write function, raising it in the read one was
causing the irq to get raised twice by the linux driver that works
properly with the actual hardware. oops.
Conor Dooley (3):
hw/misc: pfsoc: add fabric clocks to ioscb
hw/riscv: pfsoc: add missing FICs as unimplemented
hw/{misc,riscv}: pfsoc: add system controller as unimplemented
hw/misc/mchp_pfsoc_ioscb.c | 78 +-
hw/misc/mchp_pfsoc_sysreg.c | 18 -
hw/riscv/microchip_pfsoc.c | 121
include/hw/misc/mchp_pfsoc_ioscb.h | 4 +
include/hw/misc/mchp_pfsoc_sysreg.h | 1 +
include/hw/riscv/microchip_pfsoc.h | 3 +
6 files changed, 167 insertions(+), 58 deletions(-)
--
2.37.2
[PATCH v3 1/3] hw/misc: pfsoc: add fabric clocks to ioscb
From: Conor Dooley
On PolarFire SoC, some peripherals (eg the PCI root port) are clocked by
"Clock Conditioning Circuitry" in the FPGA. The specific clock depends
on the FPGA bitstream & can be locked to one particular {D,P}LL - in the
Icicle Kit Reference Design v2022.09 or later this is/will be the case.
Linux v6.1+ will have a driver for this peripheral and devicetrees that
previously relied on "fixed-frequency" clock nodes have been switched
over to clock-controller nodes. The IOSCB region is represented in QEMU,
but the specific region of it that the CCCs occupy has not so v6.1-rcN
kernels fail to boot in QEMU.
Add the regions as unimplemented so that the status-quo in terms of boot
is maintained.
Acked-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/misc/mchp_pfsoc_ioscb.c | 6 ++
include/hw/misc/mchp_pfsoc_ioscb.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
index f4fd55a0e5..f976e42f72 100644
--- a/hw/misc/mchp_pfsoc_ioscb.c
+++ b/hw/misc/mchp_pfsoc_ioscb.c
@@ -33,6 +33,7 @@
*/
#define IOSCB_WHOLE_REG_SIZE0x1000
#define IOSCB_SUBMOD_REG_SIZE 0x1000
+#define IOSCB_CCC_REG_SIZE 0x200
/*
* There are many sub-modules in the IOSCB module.
@@ -45,6 +46,7 @@
#define IOSCB_LANE23_BASE 0x0651
#define IOSCB_CTRL_BASE 0x0702
#define IOSCB_CFG_BASE 0x0708
+#define IOSCB_CCC_BASE 0x0800
#define IOSCB_PLL_MSS_BASE 0x0E001000
#define IOSCB_CFM_MSS_BASE 0x0E002000
#define IOSCB_PLL_DDR_BASE 0x0E01
@@ -168,6 +170,10 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
Error **errp)
"mchp.pfsoc.ioscb.cfg", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_CFG_BASE, >cfg);
+memory_region_init_io(>ccc, OBJECT(s), _pfsoc_dummy_ops, s,
+ "mchp.pfsoc.ioscb.ccc", IOSCB_CCC_REG_SIZE);
+memory_region_add_subregion(>container, IOSCB_CCC_BASE, >ccc);
+
memory_region_init_io(>pll_mss, OBJECT(s), _pfsoc_pll_ops, s,
"mchp.pfsoc.ioscb.pll_mss", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_PLL_MSS_BASE,
>pll_mss);
diff --git a/include/hw/misc/mchp_pfsoc_ioscb.h
b/include/hw/misc/mchp_pfsoc_ioscb.h
index 9235523e33..687b213742 100644
--- a/include/hw/misc/mchp_pfsoc_ioscb.h
+++ b/include/hw/misc/mchp_pfsoc_ioscb.h
@@ -30,6 +30,7 @@ typedef struct MchpPfSoCIoscbState {
MemoryRegion lane23;
MemoryRegion ctrl;
MemoryRegion cfg;
+MemoryRegion ccc;
MemoryRegion pll_mss;
MemoryRegion cfm_mss;
MemoryRegion pll_ddr;
--
2.37.2
Re: [PATCH v2 3/3] hw/{misc, riscv}: pfsoc: add system controller as unimplemented
On Sat, Nov 12, 2022 at 01:34:15PM +, Conor Dooley wrote:
> From: Conor Dooley
>
> The system controller on PolarFire SoC is access via a mailbox. The
> control registers for this mailbox lie in the "IOSCB" region & the
> interrupt is cleared via write to the "SYSREG" region. It also has a
> QSPI controller, usually connected to a flash chip, that is used for
> storing FPGA bitstreams and used for In-Application Programming (IAP).
>
> Linux has an implementation of the system controller, through which the
> hwrng is accessed, leading to load/store access faults.
>
> Add the QSPI as unimplemented and a very basic (effectively
> unimplemented) version of the system controller's mailbox. Rather than
> purely marking the regions as unimplemented, service the mailbox
> requests by reporting failures and raising the interrupt so a guest can
> better handle the lack of support.
>
> Signed-off-by: Conor Dooley
> ---
> hw/misc/mchp_pfsoc_ioscb.c | 59 -
> hw/misc/mchp_pfsoc_sysreg.c | 19 --
> hw/riscv/microchip_pfsoc.c | 6 +++
> include/hw/misc/mchp_pfsoc_ioscb.h | 3 ++
> include/hw/misc/mchp_pfsoc_sysreg.h | 1 +
> include/hw/riscv/microchip_pfsoc.h | 1 +
> 6 files changed, 83 insertions(+), 6 deletions(-)
>
> diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
> index f976e42f72..d7f27b4402 100644
> --- a/hw/misc/mchp_pfsoc_ioscb.c
> +++ b/hw/misc/mchp_pfsoc_ioscb.c
> @@ -24,6 +24,7 @@
> #include "qemu/bitops.h"
> #include "qemu/log.h"
> #include "qapi/error.h"
> +#include "hw/irq.h"
> #include "hw/sysbus.h"
> #include "hw/misc/mchp_pfsoc_ioscb.h"
>
> @@ -34,6 +35,9 @@
> #define IOSCB_WHOLE_REG_SIZE0x1000
> #define IOSCB_SUBMOD_REG_SIZE 0x1000
> #define IOSCB_CCC_REG_SIZE 0x200
> +#define IOSCB_CTRL_REG_SIZE 0x800
> +#define IOSCB_QSPIXIP_REG_SIZE 0x200
> +
>
> /*
> * There are many sub-modules in the IOSCB module.
> @@ -45,6 +49,8 @@
> #define IOSCB_LANE01_BASE 0x0650
> #define IOSCB_LANE23_BASE 0x0651
> #define IOSCB_CTRL_BASE 0x0702
> +#define IOSCB_QSPIXIP_BASE 0x07020100
> +#define IOSCB_MAILBOX_BASE 0x07020800
> #define IOSCB_CFG_BASE 0x0708
> #define IOSCB_CCC_BASE 0x0800
> #define IOSCB_PLL_MSS_BASE 0x0E001000
> @@ -143,6 +149,45 @@ static const MemoryRegionOps mchp_pfsoc_io_calib_ddr_ops
> = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> +#define SERVICES_SR 0x54
> +
> +static uint64_t mchp_pfsoc_ctrl_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> +MchpPfSoCIoscbState *s = opaque;
> +uint32_t val = 0;
> +
> +switch (offset) {
> +case SERVICES_SR:
> +/*
> + * Although some services have no error codes, most do. All services
> + * that do implement errors, begin their error codes at 1. Treat all
> + * service requests as failures & return 1.
> + * See the "PolarFire® FPGA and PolarFire SoC FPGA System Services"
> + * user guide for more information on service error codes.
> + */
> +val = 1;
Another issue I just spotted here, this should not be returning 1, but
rather 1 << 16. The status bits are 31:16 & I was just getting lucky
b/c of something wrong with the Linux driver exercising it!
> +qemu_irq_raise(s->irq);
> +break;
> +default:
> +qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read "
> + "(size %d, offset 0x%" HWADDR_PRIx ")\n",
> + __func__, size, offset);
> +}
> +
> +return val;
> +}
> +
> +/*
> + * use the dummy write, since we are always going to report a failed message
> + * and therefore do not care what service is actually requested
> + */
> +static const MemoryRegionOps mchp_pfsoc_ctrl_ops = {
> +.read = mchp_pfsoc_ctrl_read,
> +.write = mchp_pfsoc_dummy_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> static void mchp_pfsoc_ioscb_realize(DeviceState *dev, Error **errp)
> {
> MchpPfSoCIoscbState *s = MCHP_PFSOC_IOSCB(dev);
> @@ -162,10 +207,18 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
> Error **errp)
>"mchp.pfsoc.ioscb.lane23", IOSCB_SUBMOD_REG_SIZE);
> memory_region_add_subregion(>container, IOSCB_LANE23_BASE,
> >lane23);
>
Re: [PATCH v2 3/3] hw/{misc, riscv}: pfsoc: add system controller as unimplemented
On Sun, Nov 13, 2022 at 08:30:42PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Conor,
>
> On 12/11/22 14:34, Conor Dooley wrote:
> > From: Conor Dooley
> >
> > The system controller on PolarFire SoC is access via a mailbox. The
> > control registers for this mailbox lie in the "IOSCB" region & the
> > interrupt is cleared via write to the "SYSREG" region. It also has a
> > QSPI controller, usually connected to a flash chip, that is used for
> > storing FPGA bitstreams and used for In-Application Programming (IAP).
> >
> > Linux has an implementation of the system controller, through which the
> > hwrng is accessed, leading to load/store access faults.
> >
> > Add the QSPI as unimplemented and a very basic (effectively
> > unimplemented) version of the system controller's mailbox. Rather than
> > purely marking the regions as unimplemented, service the mailbox
> > requests by reporting failures and raising the interrupt so a guest can
> > better handle the lack of support.
> >
> > Signed-off-by: Conor Dooley
> > ---
> > hw/misc/mchp_pfsoc_ioscb.c | 59 -
> > hw/misc/mchp_pfsoc_sysreg.c | 19 --
> > hw/riscv/microchip_pfsoc.c | 6 +++
> > include/hw/misc/mchp_pfsoc_ioscb.h | 3 ++
> > include/hw/misc/mchp_pfsoc_sysreg.h | 1 +
> > include/hw/riscv/microchip_pfsoc.h | 1 +
> > 6 files changed, 83 insertions(+), 6 deletions(-)
>
> > @@ -52,10 +54,18 @@ static uint64_t mchp_pfsoc_sysreg_read(void *opaque,
> > hwaddr offset,
> > static void mchp_pfsoc_sysreg_write(void *opaque, hwaddr offset,
> > uint64_t value, unsigned size)
> > {
> > -qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write "
> > - "(size %d, value 0x%" PRIx64
> > - ", offset 0x%" HWADDR_PRIx ")\n",
> > - __func__, size, value, offset);
> > +MchpPfSoCSysregState *s = opaque;
> > +qemu_irq_lower(s->irq);
>
> Is this always lowered IRQ line wanted? ...
>
> > +switch (offset) {
> > +case MESSAGE_INT:
> > +qemu_irq_lower(s->irq);
>
> ... since we do it here.
Probably just me pressing the y key instead of the d one.
I'll sort that out for v3, thanks!
> > +break;
> > +default:
> > +qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write "
> > + "(size %d, value 0x%" PRIx64
> > + ", offset 0x%" HWADDR_PRIx ")\n",
> > + __func__, size, value, offset);
> > +}
> > }
>
>
[PATCH v2 0/3] Add (more) missing PolarFire SoC io regions
From: Conor Dooley
Hey all,
But of a v2 of what I sent the other day [0]..
Apart from DDR (see [1]), these should be the last bits needed to get
recent Linux kernels booting again for Icicle/PolarFire SoC. Previously,
I had been disabling the hwrng and PCI but I keep forgetting that is
required and decided to fix that.
I'm not entirely sure if I have done some sort of no-no thing by
registering the same interrupt with both the IOSCB and SYSREG regions.
The interrupt is raised after the system controller handles a service
via the mailbox. The mailbox's status, control and mailbox registers
are all part of the IOSCB region. It's cleared by a write to a register
in the SYSREG region.
Since my goal here is to add the regions/peripherals without actually
implementing them so that Linux etc, I'm just raising an interrupt
once a guest requests a service & reporting a status indicating that the
service request failed.
Thanks,
Conor.
0 -
https://lore.kernel.org/qemu-devel/20221109190849.1556711-1-co...@kernel.org/
1 - https://lore.kernel.org/all/Y2+dUCpd8OP52%2FDJ@spud/
Conor Dooley (3):
hw/misc/pfsoc: add fabric clocks to ioscb
hw/riscv: pfsoc: add missing FICs as unimplemented
hw/{misc,riscv}: pfsoc: add system controller as unimplemented
hw/misc/mchp_pfsoc_ioscb.c | 65 ++-
hw/misc/mchp_pfsoc_sysreg.c | 19 -
hw/riscv/microchip_pfsoc.c | 121
include/hw/misc/mchp_pfsoc_ioscb.h | 4 +
include/hw/misc/mchp_pfsoc_sysreg.h | 1 +
include/hw/riscv/microchip_pfsoc.h | 3 +
6 files changed, 155 insertions(+), 58 deletions(-)
--
2.37.2
[PATCH v2 3/3] hw/{misc, riscv}: pfsoc: add system controller as unimplemented
From: Conor Dooley
The system controller on PolarFire SoC is access via a mailbox. The
control registers for this mailbox lie in the "IOSCB" region & the
interrupt is cleared via write to the "SYSREG" region. It also has a
QSPI controller, usually connected to a flash chip, that is used for
storing FPGA bitstreams and used for In-Application Programming (IAP).
Linux has an implementation of the system controller, through which the
hwrng is accessed, leading to load/store access faults.
Add the QSPI as unimplemented and a very basic (effectively
unimplemented) version of the system controller's mailbox. Rather than
purely marking the regions as unimplemented, service the mailbox
requests by reporting failures and raising the interrupt so a guest can
better handle the lack of support.
Signed-off-by: Conor Dooley
---
hw/misc/mchp_pfsoc_ioscb.c | 59 -
hw/misc/mchp_pfsoc_sysreg.c | 19 --
hw/riscv/microchip_pfsoc.c | 6 +++
include/hw/misc/mchp_pfsoc_ioscb.h | 3 ++
include/hw/misc/mchp_pfsoc_sysreg.h | 1 +
include/hw/riscv/microchip_pfsoc.h | 1 +
6 files changed, 83 insertions(+), 6 deletions(-)
diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
index f976e42f72..d7f27b4402 100644
--- a/hw/misc/mchp_pfsoc_ioscb.c
+++ b/hw/misc/mchp_pfsoc_ioscb.c
@@ -24,6 +24,7 @@
#include "qemu/bitops.h"
#include "qemu/log.h"
#include "qapi/error.h"
+#include "hw/irq.h"
#include "hw/sysbus.h"
#include "hw/misc/mchp_pfsoc_ioscb.h"
@@ -34,6 +35,9 @@
#define IOSCB_WHOLE_REG_SIZE0x1000
#define IOSCB_SUBMOD_REG_SIZE 0x1000
#define IOSCB_CCC_REG_SIZE 0x200
+#define IOSCB_CTRL_REG_SIZE 0x800
+#define IOSCB_QSPIXIP_REG_SIZE 0x200
+
/*
* There are many sub-modules in the IOSCB module.
@@ -45,6 +49,8 @@
#define IOSCB_LANE01_BASE 0x0650
#define IOSCB_LANE23_BASE 0x0651
#define IOSCB_CTRL_BASE 0x0702
+#define IOSCB_QSPIXIP_BASE 0x07020100
+#define IOSCB_MAILBOX_BASE 0x07020800
#define IOSCB_CFG_BASE 0x0708
#define IOSCB_CCC_BASE 0x0800
#define IOSCB_PLL_MSS_BASE 0x0E001000
@@ -143,6 +149,45 @@ static const MemoryRegionOps mchp_pfsoc_io_calib_ddr_ops =
{
.endianness = DEVICE_LITTLE_ENDIAN,
};
+#define SERVICES_SR 0x54
+
+static uint64_t mchp_pfsoc_ctrl_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+MchpPfSoCIoscbState *s = opaque;
+uint32_t val = 0;
+
+switch (offset) {
+case SERVICES_SR:
+/*
+ * Although some services have no error codes, most do. All services
+ * that do implement errors, begin their error codes at 1. Treat all
+ * service requests as failures & return 1.
+ * See the "PolarFire® FPGA and PolarFire SoC FPGA System Services"
+ * user guide for more information on service error codes.
+ */
+val = 1;
+qemu_irq_raise(s->irq);
+break;
+default:
+qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read "
+ "(size %d, offset 0x%" HWADDR_PRIx ")\n",
+ __func__, size, offset);
+}
+
+return val;
+}
+
+/*
+ * use the dummy write, since we are always going to report a failed message
+ * and therefore do not care what service is actually requested
+ */
+static const MemoryRegionOps mchp_pfsoc_ctrl_ops = {
+.read = mchp_pfsoc_ctrl_read,
+.write = mchp_pfsoc_dummy_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
static void mchp_pfsoc_ioscb_realize(DeviceState *dev, Error **errp)
{
MchpPfSoCIoscbState *s = MCHP_PFSOC_IOSCB(dev);
@@ -162,10 +207,18 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
Error **errp)
"mchp.pfsoc.ioscb.lane23", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_LANE23_BASE, >lane23);
-memory_region_init_io(>ctrl, OBJECT(s), _pfsoc_dummy_ops, s,
- "mchp.pfsoc.ioscb.ctrl", IOSCB_SUBMOD_REG_SIZE);
+memory_region_init_io(>ctrl, OBJECT(s), _pfsoc_ctrl_ops, s,
+ "mchp.pfsoc.ioscb.ctrl", IOSCB_CTRL_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_CTRL_BASE, >ctrl);
+memory_region_init_io(>qspixip, OBJECT(s), _pfsoc_dummy_ops, s,
+ "mchp.pfsoc.ioscb.qspixip", IOSCB_QSPIXIP_REG_SIZE);
+memory_region_add_subregion(>container, IOSCB_QSPIXIP_BASE,
>qspixip);
+
+memory_region_init_io(>mailbox, OBJECT(s), _pfsoc_dummy_ops, s,
+ "mchp.pfsoc.ioscb.mailbox", IOSCB_SUBMOD_REG_SIZE);
+memory_region_add_subregion(>container, IOS
[PATCH v2 2/3] hw/riscv: pfsoc: add missing FICs as unimplemented
From: Conor Dooley
The Fabric Interconnect Controllers provide interfaces between the FPGA
fabric and the core complex. There are 5 FICs on PolarFire SoC, numbered
0 through 4. FIC2 is an AXI4 slave interface from the FPGA fabric and
does not show up on the MSS memory map. FIC4 is dedicated to the User
Crypto Processor and does not show up on the MSS memory map either.
FIC 0, 1 & 3 do show up in the MSS memory map and neither FICs 0 or 1
are represented in QEMU, leading to load access violations while booting
Linux for Icicle if PCIe is enabled as the root port is connected via
either FIC 0 or 1.
Signed-off-by: Conor Dooley
---
hw/riscv/microchip_pfsoc.c | 115 -
include/hw/riscv/microchip_pfsoc.h | 2 +
2 files changed, 65 insertions(+), 52 deletions(-)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index a821263d4f..2a24e3437a 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -86,58 +86,61 @@
* describes the complete IOSCB modules memory maps
*/
static const MemMapEntry microchip_pfsoc_memmap[] = {
-[MICROCHIP_PFSOC_RSVD0] = {0x0, 0x100 },
-[MICROCHIP_PFSOC_DEBUG] = { 0x100, 0xf00 },
-[MICROCHIP_PFSOC_E51_DTIM] ={ 0x100, 0x2000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT0] ={ 0x170, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT1] ={ 0x1701000, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT2] ={ 0x1702000, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT3] ={ 0x1703000, 0x1000 },
-[MICROCHIP_PFSOC_BUSERR_UNIT4] ={ 0x1704000, 0x1000 },
-[MICROCHIP_PFSOC_CLINT] = { 0x200,0x1 },
-[MICROCHIP_PFSOC_L2CC] ={ 0x201, 0x1000 },
-[MICROCHIP_PFSOC_DMA] = { 0x300, 0x10 },
-[MICROCHIP_PFSOC_L2LIM] = { 0x800, 0x200 },
-[MICROCHIP_PFSOC_PLIC] ={ 0xc00, 0x400 },
-[MICROCHIP_PFSOC_MMUART0] = { 0x2000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG0] = { 0x20001000, 0x1000 },
-[MICROCHIP_PFSOC_SYSREG] = { 0x20002000, 0x2000 },
-[MICROCHIP_PFSOC_AXISW] = { 0x20004000, 0x1000 },
-[MICROCHIP_PFSOC_MPUCFG] = { 0x20005000, 0x1000 },
-[MICROCHIP_PFSOC_FMETER] = { 0x20006000, 0x1000 },
-[MICROCHIP_PFSOC_DDR_SGMII_PHY] = { 0x20007000, 0x1000 },
-[MICROCHIP_PFSOC_EMMC_SD] = { 0x20008000, 0x1000 },
-[MICROCHIP_PFSOC_DDR_CFG] = { 0x2008,0x4 },
-[MICROCHIP_PFSOC_MMUART1] = { 0x2010, 0x1000 },
-[MICROCHIP_PFSOC_MMUART2] = { 0x20102000, 0x1000 },
-[MICROCHIP_PFSOC_MMUART3] = { 0x20104000, 0x1000 },
-[MICROCHIP_PFSOC_MMUART4] = { 0x20106000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG1] = { 0x20101000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG2] = { 0x20103000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG3] = { 0x20105000, 0x1000 },
-[MICROCHIP_PFSOC_WDOG4] = { 0x20106000, 0x1000 },
-[MICROCHIP_PFSOC_SPI0] ={ 0x20108000, 0x1000 },
-[MICROCHIP_PFSOC_SPI1] ={ 0x20109000, 0x1000 },
-[MICROCHIP_PFSOC_I2C0] ={ 0x2010a000, 0x1000 },
-[MICROCHIP_PFSOC_I2C1] ={ 0x2010b000, 0x1000 },
-[MICROCHIP_PFSOC_CAN0] ={ 0x2010c000, 0x1000 },
-[MICROCHIP_PFSOC_CAN1] ={ 0x2010d000, 0x1000 },
-[MICROCHIP_PFSOC_GEM0] ={ 0x2011, 0x2000 },
-[MICROCHIP_PFSOC_GEM1] ={ 0x20112000, 0x2000 },
-[MICROCHIP_PFSOC_GPIO0] = { 0x2012, 0x1000 },
-[MICROCHIP_PFSOC_GPIO1] = { 0x20121000, 0x1000 },
-[MICROCHIP_PFSOC_GPIO2] = { 0x20122000, 0x1000 },
-[MICROCHIP_PFSOC_RTC] = { 0x20124000, 0x1000 },
-[MICROCHIP_PFSOC_ENVM_CFG] ={ 0x2020, 0x1000 },
-[MICROCHIP_PFSOC_ENVM_DATA] = { 0x2022,0x2 },
-[MICROCHIP_PFSOC_USB] = { 0x20201000, 0x1000 },
-[MICROCHIP_PFSOC_QSPI_XIP] ={ 0x2100, 0x100 },
-[MICROCHIP_PFSOC_IOSCB] = { 0x3000, 0x1000 },
-[MICROCHIP_PFSOC_FABRIC_FIC3] = { 0x4000, 0x2000 },
-[MICROCHIP_PFSOC_DRAM_LO] = { 0x8000, 0x4000 },
-[MICROCHIP_PFSOC_DRAM_LO_ALIAS] = { 0xc000, 0x4000 },
-[MICROCHIP_PFSOC_DRAM_HI] = { 0x10,0x0 },
-[MICROCHIP_PFSOC_DRAM_HI_ALIAS] = { 0x14,0x0 },
+[MICROCHIP_PFSOC_RSVD0] = {0x0,0x100 },
+[MICROCHIP_PFSOC_DEBUG] = { 0x100,0xf00 },
+[MICROCHIP_PFSOC_E51_DTIM] ={ 0x100, 0x2000 },
+[MICROCHIP_PFSOC_BUSERR_UNIT0] ={ 0x170, 0x
[PATCH v2 1/3] hw/misc/pfsoc: add fabric clocks to ioscb
From: Conor Dooley
On PolarFire SoC, some peripherals (eg the PCI root port) are clocked by
"Clock Conditioning Circuitry" in the FPGA. The specific clock depends
on the FPGA bitstream & can be locked to one particular {D,P}LL - in the
Icicle Kit Reference Design v2022.09 or later this is/will be the case.
Linux v6.1+ will have a driver for this peripheral and devicetrees that
previously relied on "fixed-frequency" clock nodes have been switched
over to clock-controller nodes. The IOSCB region is represented in QEMU,
but the specific region of it that the CCCs occupy has not so v6.1-rcN
kernels fail to boot in QEMU.
Add the regions as unimplemented so that the status-quo in terms of boot
is maintained.
Signed-off-by: Conor Dooley
---
hw/misc/mchp_pfsoc_ioscb.c | 6 ++
include/hw/misc/mchp_pfsoc_ioscb.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
index f4fd55a0e5..f976e42f72 100644
--- a/hw/misc/mchp_pfsoc_ioscb.c
+++ b/hw/misc/mchp_pfsoc_ioscb.c
@@ -33,6 +33,7 @@
*/
#define IOSCB_WHOLE_REG_SIZE0x1000
#define IOSCB_SUBMOD_REG_SIZE 0x1000
+#define IOSCB_CCC_REG_SIZE 0x200
/*
* There are many sub-modules in the IOSCB module.
@@ -45,6 +46,7 @@
#define IOSCB_LANE23_BASE 0x0651
#define IOSCB_CTRL_BASE 0x0702
#define IOSCB_CFG_BASE 0x0708
+#define IOSCB_CCC_BASE 0x0800
#define IOSCB_PLL_MSS_BASE 0x0E001000
#define IOSCB_CFM_MSS_BASE 0x0E002000
#define IOSCB_PLL_DDR_BASE 0x0E01
@@ -168,6 +170,10 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
Error **errp)
"mchp.pfsoc.ioscb.cfg", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_CFG_BASE, >cfg);
+memory_region_init_io(>ccc, OBJECT(s), _pfsoc_dummy_ops, s,
+ "mchp.pfsoc.ioscb.ccc", IOSCB_CCC_REG_SIZE);
+memory_region_add_subregion(>container, IOSCB_CCC_BASE, >ccc);
+
memory_region_init_io(>pll_mss, OBJECT(s), _pfsoc_pll_ops, s,
"mchp.pfsoc.ioscb.pll_mss", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_PLL_MSS_BASE,
>pll_mss);
diff --git a/include/hw/misc/mchp_pfsoc_ioscb.h
b/include/hw/misc/mchp_pfsoc_ioscb.h
index 9235523e33..687b213742 100644
--- a/include/hw/misc/mchp_pfsoc_ioscb.h
+++ b/include/hw/misc/mchp_pfsoc_ioscb.h
@@ -30,6 +30,7 @@ typedef struct MchpPfSoCIoscbState {
MemoryRegion lane23;
MemoryRegion ctrl;
MemoryRegion cfg;
+MemoryRegion ccc;
MemoryRegion pll_mss;
MemoryRegion cfm_mss;
MemoryRegion pll_ddr;
--
2.37.2
Re: [PATCH] hw/misc/pfsoc: add fabric clocks to ioscb
On Sat, Nov 12, 2022 at 08:37:38AM +0800, Bin Meng wrote: > Hi Conor, > > On Sat, Nov 12, 2022 at 8:31 AM Conor Dooley wrote: > > > > On Thu, Nov 10, 2022 at 12:18:44AM +0100, Philippe Mathieu-Daudé wrote: > > > Hi Conor, > > > > > > On 9/11/22 20:08, Conor Dooley wrote: > > > > From: Conor Dooley > > > > > > > > @@ -168,6 +170,10 @@ static void mchp_pfsoc_ioscb_realize(DeviceState > > > > *dev, Error **errp) > > > > "mchp.pfsoc.ioscb.cfg", > > > > IOSCB_SUBMOD_REG_SIZE); > > > > memory_region_add_subregion(>container, IOSCB_CFG_BASE, > > > > >cfg); > > > > +memory_region_init_io(>ccc, OBJECT(s), _pfsoc_dummy_ops, s, > > > > + "mchp.pfsoc.ioscb.ccc", IOSCB_CCC_REG_SIZE); > > > > +memory_region_add_subregion(>container, IOSCB_CCC_BASE, > > > > >ccc); > > > > > > Unrelated but using the TYPE_UNIMPLEMENTED_DEVICE would ease tracing all > > > these block accesses, as the block name would appear before the > > > address/size. See for example aspeed_mmio_map_unimplemented(); > > > > Certainly looks like a nice idea, and I gave it a go but kept running > > into issues due to my lack of understanding of QEMU :) I'm going to add > > this to my todo pile - while I have a v2 of this lined up, I'd rather > > not hold up adding the regions that prevent booting Linux etc as I > > fumble around trying to understand the hierarchy of devices required to > > set up something similar to your aspeed example. > > > > Do you plan to bring QEMU support to the latest MSS_LINUX configuration [1] "Yes". Our goal is to merge both the LINUX and BAREMETAL configurations in an upcoming reference design release. Notably absent from anything that I have sent here is any changes to the DDR configuration (and that and how the PCI root port is connected to the MSS are the only real differences between the two). Currently, the LINUX one has 2 GiB of DDR at 0x10__ & that's what the vendor kernel uses. None of upstream Linux, U-Boot or QEMU support that configuration. The baremetal one has 1 GiB at 0x8000_ and 1 GiB at 0x10__. When the two are merged, it'll be 1 GiB at 0x8000_ and 1 GiB at 0x10_4000_ - there's currently a v2022.10 reference design job file on [1] that's got this configuration but we are waiting for a corresponding release of Libero to properly release the tcl scripts etc. We're upstreaming U-Boot and Linux support for that configuration at the moment - but it's just a dts change there so no real concern about breaking any backwards compat as the older devicetrees will continue to work. > Currently QEMU is supporting the MSS_BAREMETAL configuration. Do you > think it makes sense to support both? > [1] https://github.com/polarfire-soc/icicle-kit-reference-design I was kinda hoping to leave that part of things as-is for now and wait for the merged configuration. My main question with that is: do the older reference design configurations need to remain supported? The PCI root port stuff likely doesn't matter since it's not modelled (yet) by QEMU anyway but the DDR bit is going to be incompatible. The addresses at which DDR lies are controlled by the seg registers. These are briefly documented in the TRM (4.5 Segmentation Blocks) but IMO pretty badly explained there. IIUC, for bare metal applications that's set by the HAL from the XML exported by MSS configurator & for anything started via the HSS, the HSS does it instead. I was thinking something like defaulting the DDR configuration to the new, merged configuration & then if someone writes to the seg registers (which, IIUC, a bare-metal app does) changing the addresses at which QEMU places the DDR at runtime. That's what the hardware does, but I have put approximately zero thought into how to implement that. Without something like that, idk how we'd keep both newer and older reference designs working in QEMU. > Do you plan to bring QEMU support to the latest MSS_LINUX configuration Either way, any plans are dependant on me finding the time. I'm mostly just upstreaming the small changes that I need to make so that QEMU remains usable as a debugging tool for Linux stuff. Thanks, Conor.
Re: [PATCH] hw/misc/pfsoc: add fabric clocks to ioscb
On Thu, Nov 10, 2022 at 12:18:44AM +0100, Philippe Mathieu-Daudé wrote: > Hi Conor, > > On 9/11/22 20:08, Conor Dooley wrote: > > From: Conor Dooley > > > > @@ -168,6 +170,10 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev, > > Error **errp) > > "mchp.pfsoc.ioscb.cfg", IOSCB_SUBMOD_REG_SIZE); > > memory_region_add_subregion(>container, IOSCB_CFG_BASE, >cfg); > > +memory_region_init_io(>ccc, OBJECT(s), _pfsoc_dummy_ops, s, > > + "mchp.pfsoc.ioscb.ccc", IOSCB_CCC_REG_SIZE); > > +memory_region_add_subregion(>container, IOSCB_CCC_BASE, >ccc); > > Unrelated but using the TYPE_UNIMPLEMENTED_DEVICE would ease tracing all > these block accesses, as the block name would appear before the > address/size. See for example aspeed_mmio_map_unimplemented(); Certainly looks like a nice idea, and I gave it a go but kept running into issues due to my lack of understanding of QEMU :) I'm going to add this to my todo pile - while I have a v2 of this lined up, I'd rather not hold up adding the regions that prevent booting Linux etc as I fumble around trying to understand the hierarchy of devices required to set up something similar to your aspeed example. Thanks, Conor.
Re: [PATCH] hw/misc/pfsoc: add fabric clocks to ioscb
On Thu, Nov 10, 2022 at 12:18:44AM +0100, Philippe Mathieu-Daudé wrote:
> On 9/11/22 20:08, Conor Dooley wrote:
> > From: Conor Dooley
> >
> > On PolarFire SoC, some peripherals (eg the PCI root port) are clocked by
> > "Clock Conditioning Circuitry" in the FPGA. The specific clock depends
> > on the FPGA bitstream & can be locked to one particular {D,P}LL - in the
> > Icicle Kit Reference Design v2022.09 or later this is/will be the case.
> >
> > Linux v6.1+ will have a driver for this peripheral and devicetrees that
> > previously relied on "fixed-frequency" clock nodes have been switched
> > over to clock-controller nodes. The IOSCB region is represented in QEMU,
> > but the specific region of it that the CCCs occupy has not so v6.1-rcN
> > kernels fail to boot in QEMU.
> >
> > Add the regions as unimplemented so that the status-quo in terms of boot
> > is maintained.
> >
> > Signed-off-by: Conor Dooley
> > ---
> > The last line there is a white lie. v6.1-rcN has both v2022.09 and
> > v2022.10 reference design changes. This patch only accounts for the
> > v2022.09 changes. The FPGA design is a moving target and I am not
> > really sure how to handle that in QEMU. For v2022.10 a bunch of stuff
> > got changed, including the addresses that DDR lies at which I am not
> > sure how to handle yet.
> >
> > That puts my todo list of broken things to:
> > - MMC (only direct kernel boot works), pre v2022.09 reference issue
>
> How do you start without 'direct kernel boot'?
You used to be able to load the "bios" etc and follow the boot flow [0].
This no longer works, and has not for at least a year. I assume it still
works if you check out the (fossilised) versions mentioned in that doc.
Think I said it last time I sent patches, but we had some floating around
internally that I know /did/ work at some point about this time last year
but I was never able to figure out the correct alignment of the stars to
get working myself. It required pretty decent changes to the sdhci driver,
which, I'm hoping cease to be required with the v2022.10 reference
design that I mentioned else where in this patch.
But one problem at a time ;)
0 - https://www.qemu.org/docs/master/system/riscv/microchip-icicle-kit.html
> > - PCI root port address, address changed in v2022.09 but from a cursory
> >check, I didn't see any PCI support in the first place. It's connected
> >to a FIC, so I think it can just be made into an unimplemented region.
> > - DDR address changes, 2022.10 issue. Looks like a straightforward
> >change to hw/riscv/pfsoc.c but I don't think it'll be backwards
> >compatible.
> > - hwrng breaks boot. Tipping away at this one, hopefully I'll have a fix
> >for it soon. Need to implement the irq side of the mailbox for it.
> >
> > I'll send some more patches as I work through them.
> >
> > hw/misc/mchp_pfsoc_ioscb.c | 6 ++
> > include/hw/misc/mchp_pfsoc_ioscb.h | 1 +
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
> > index f4fd55a0e5..f976e42f72 100644
> > --- a/hw/misc/mchp_pfsoc_ioscb.c
> > +++ b/hw/misc/mchp_pfsoc_ioscb.c
> > @@ -33,6 +33,7 @@
> >*/
> > #define IOSCB_WHOLE_REG_SIZE0x1000
> > #define IOSCB_SUBMOD_REG_SIZE 0x1000
> > +#define IOSCB_CCC_REG_SIZE 0x200
> > /*
> >* There are many sub-modules in the IOSCB module.
> > @@ -45,6 +46,7 @@
> > #define IOSCB_LANE23_BASE 0x0651
> > #define IOSCB_CTRL_BASE 0x0702
> > #define IOSCB_CFG_BASE 0x0708
> > +#define IOSCB_CCC_BASE 0x0800
> > #define IOSCB_PLL_MSS_BASE 0x0E001000
> > #define IOSCB_CFM_MSS_BASE 0x0E002000
> > #define IOSCB_PLL_DDR_BASE 0x0E01
> > @@ -168,6 +170,10 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
> > Error **errp)
> > "mchp.pfsoc.ioscb.cfg", IOSCB_SUBMOD_REG_SIZE);
> > memory_region_add_subregion(>container, IOSCB_CFG_BASE, >cfg);
> > +memory_region_init_io(>ccc, OBJECT(s), _pfsoc_dummy_ops, s,
> > + "mchp.pfsoc.ioscb.ccc", IOSCB_CCC_REG_SIZE);
> > +memory_region_add_subregion(>container, IOSCB_CCC_BASE, >ccc);
>
> Unrelated but using the TYPE_UNIMPLEMENTED_DEVICE would ease tracing all
> these block accesses, as the block name would appear before the
> address/size. See for example aspeed_mmio_map_unimplemented
[PATCH] hw/misc/pfsoc: add fabric clocks to ioscb
From: Conor Dooley
On PolarFire SoC, some peripherals (eg the PCI root port) are clocked by
"Clock Conditioning Circuitry" in the FPGA. The specific clock depends
on the FPGA bitstream & can be locked to one particular {D,P}LL - in the
Icicle Kit Reference Design v2022.09 or later this is/will be the case.
Linux v6.1+ will have a driver for this peripheral and devicetrees that
previously relied on "fixed-frequency" clock nodes have been switched
over to clock-controller nodes. The IOSCB region is represented in QEMU,
but the specific region of it that the CCCs occupy has not so v6.1-rcN
kernels fail to boot in QEMU.
Add the regions as unimplemented so that the status-quo in terms of boot
is maintained.
Signed-off-by: Conor Dooley
---
The last line there is a white lie. v6.1-rcN has both v2022.09 and
v2022.10 reference design changes. This patch only accounts for the
v2022.09 changes. The FPGA design is a moving target and I am not
really sure how to handle that in QEMU. For v2022.10 a bunch of stuff
got changed, including the addresses that DDR lies at which I am not
sure how to handle yet.
That puts my todo list of broken things to:
- MMC (only direct kernel boot works), pre v2022.09 reference issue
- PCI root port address, address changed in v2022.09 but from a cursory
check, I didn't see any PCI support in the first place. It's connected
to a FIC, so I think it can just be made into an unimplemented region.
- DDR address changes, 2022.10 issue. Looks like a straightforward
change to hw/riscv/pfsoc.c but I don't think it'll be backwards
compatible.
- hwrng breaks boot. Tipping away at this one, hopefully I'll have a fix
for it soon. Need to implement the irq side of the mailbox for it.
I'll send some more patches as I work through them.
hw/misc/mchp_pfsoc_ioscb.c | 6 ++
include/hw/misc/mchp_pfsoc_ioscb.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c
index f4fd55a0e5..f976e42f72 100644
--- a/hw/misc/mchp_pfsoc_ioscb.c
+++ b/hw/misc/mchp_pfsoc_ioscb.c
@@ -33,6 +33,7 @@
*/
#define IOSCB_WHOLE_REG_SIZE0x1000
#define IOSCB_SUBMOD_REG_SIZE 0x1000
+#define IOSCB_CCC_REG_SIZE 0x200
/*
* There are many sub-modules in the IOSCB module.
@@ -45,6 +46,7 @@
#define IOSCB_LANE23_BASE 0x0651
#define IOSCB_CTRL_BASE 0x0702
#define IOSCB_CFG_BASE 0x0708
+#define IOSCB_CCC_BASE 0x0800
#define IOSCB_PLL_MSS_BASE 0x0E001000
#define IOSCB_CFM_MSS_BASE 0x0E002000
#define IOSCB_PLL_DDR_BASE 0x0E01
@@ -168,6 +170,10 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev,
Error **errp)
"mchp.pfsoc.ioscb.cfg", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_CFG_BASE, >cfg);
+memory_region_init_io(>ccc, OBJECT(s), _pfsoc_dummy_ops, s,
+ "mchp.pfsoc.ioscb.ccc", IOSCB_CCC_REG_SIZE);
+memory_region_add_subregion(>container, IOSCB_CCC_BASE, >ccc);
+
memory_region_init_io(>pll_mss, OBJECT(s), _pfsoc_pll_ops, s,
"mchp.pfsoc.ioscb.pll_mss", IOSCB_SUBMOD_REG_SIZE);
memory_region_add_subregion(>container, IOSCB_PLL_MSS_BASE,
>pll_mss);
diff --git a/include/hw/misc/mchp_pfsoc_ioscb.h
b/include/hw/misc/mchp_pfsoc_ioscb.h
index 9235523e33..687b213742 100644
--- a/include/hw/misc/mchp_pfsoc_ioscb.h
+++ b/include/hw/misc/mchp_pfsoc_ioscb.h
@@ -30,6 +30,7 @@ typedef struct MchpPfSoCIoscbState {
MemoryRegion lane23;
MemoryRegion ctrl;
MemoryRegion cfg;
+MemoryRegion ccc;
MemoryRegion pll_mss;
MemoryRegion cfm_mss;
MemoryRegion pll_ddr;
--
2.37.2
[PATCH] hw/riscv: microchip_pfsoc: fix kernel panics due to missing peripherals
From: Conor Dooley
Booting using "Direct Kernel Boot" for PolarFire SoC & skipping u-boot
entirely is probably not advisable, but it does at least show signs of
life. Recent Linux kernel versions make use of peripherals that are
missing definitions in QEMU and lead to kernel panics. These issues
almost certain rear their head for other methods of booting, but I was
unable to figure out a suitable HSS version that is recent enough to
support these peripherals & works with QEMU.
With these peripherals added, booting a kernel with the following hangs
hangs waiting for the system controller's hwrng, but the kernel no
longer panics. With the Linux driver for hwrng disabled, it boots to
console.
qemu-system-riscv64 -M microchip-icicle-kit \
-m 2G -smp 5 \
-kernel $(vmlinux_bin) \
-dtb $(dtb)\
-initrd $(initramfs) \
-display none -serial null \
-serial stdio
More peripherals are added than strictly required to fix the panics in
the hopes of avoiding a replication of this problem in the future.
Some of the peripherals which are in the device tree for recent kernels
are implemented in the FPGA fabric. The eMMC/SD mux, which exists as
an unimplemented device is replaced by a wider entry. This updated
entry covers both the mux & the remainder of the FPGA fabric connected
to the MSS using Fabric Interrconnect (FIC) 3.
Link:
https://github.com/polarfire-soc/icicle-kit-reference-design#fabric-memory-map
Link:
https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/SupportingCollateral/V1_4_Register_Map.zip
Signed-off-by: Conor Dooley
---
QEMU support for PolarFire SoC seems to be fairly out of date at this
point. Running with a recent HSS, U-Boot etc doesn't work, partly due
to the unimplemented cache controller that the HSS tries to read from
(it needs to know the ways configuration now) and the rest seems to be
down to 64 bit address DMA to the sd card (not 100% on that yet).
There's some patches floating around internally that supposedly fixed
things for QEMU v6.something but I could not replicate & they're fairly
conflicty at this point. Plan is to clean them up, but no point sitting
on this patch until then as I have no ETA for that at this point.
CC: Bin Meng
CC: Palmer Dabbelt
CC: Alistair Francis
CC: Conor Dooley
CC: qemu-ri...@nongnu.org
CC: qemu-devel@nongnu.org
---
hw/riscv/microchip_pfsoc.c | 67 +++---
include/hw/riscv/microchip_pfsoc.h | 14 ++-
2 files changed, 74 insertions(+), 7 deletions(-)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 10a5d0e501..eb90a99660 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -100,8 +100,11 @@ static const MemMapEntry microchip_pfsoc_memmap[] = {
[MICROCHIP_PFSOC_L2LIM] = { 0x800, 0x200 },
[MICROCHIP_PFSOC_PLIC] ={ 0xc00, 0x400 },
[MICROCHIP_PFSOC_MMUART0] = { 0x2000, 0x1000 },
+[MICROCHIP_PFSOC_WDOG0] = { 0x20001000, 0x1000 },
[MICROCHIP_PFSOC_SYSREG] = { 0x20002000, 0x2000 },
+[MICROCHIP_PFSOC_AXISW] = { 0x20004000, 0x1000 },
[MICROCHIP_PFSOC_MPUCFG] = { 0x20005000, 0x1000 },
+[MICROCHIP_PFSOC_FMETER] = { 0x20006000, 0x1000 },
[MICROCHIP_PFSOC_DDR_SGMII_PHY] = { 0x20007000, 0x1000 },
[MICROCHIP_PFSOC_EMMC_SD] = { 0x20008000, 0x1000 },
[MICROCHIP_PFSOC_DDR_CFG] = { 0x2008,0x4 },
@@ -109,19 +112,28 @@ static const MemMapEntry microchip_pfsoc_memmap[] = {
[MICROCHIP_PFSOC_MMUART2] = { 0x20102000, 0x1000 },
[MICROCHIP_PFSOC_MMUART3] = { 0x20104000, 0x1000 },
[MICROCHIP_PFSOC_MMUART4] = { 0x20106000, 0x1000 },
+[MICROCHIP_PFSOC_WDOG1] = { 0x20101000, 0x1000 },
+[MICROCHIP_PFSOC_WDOG2] = { 0x20103000, 0x1000 },
+[MICROCHIP_PFSOC_WDOG3] = { 0x20105000, 0x1000 },
+[MICROCHIP_PFSOC_WDOG4] = { 0x20106000, 0x1000 },
[MICROCHIP_PFSOC_SPI0] ={ 0x20108000, 0x1000 },
[MICROCHIP_PFSOC_SPI1] ={ 0x20109000, 0x1000 },
+[MICROCHIP_PFSOC_I2C0] ={ 0x2010a000, 0x1000 },
[MICROCHIP_PFSOC_I2C1] ={ 0x2010b000, 0x1000 },
+[MICROCHIP_PFSOC_CAN0] ={ 0x2010c000, 0x1000 },
+[MICROCHIP_PFSOC_CAN1] ={ 0x2010d000, 0x1000 },
[MICROCHIP_PFSOC_GEM0] ={ 0x2011, 0x2000 },
[MICROCHIP_PFSOC_GEM1] ={ 0x20112000, 0x2000 },
[MICROCHIP_PFSOC_GPIO0] = { 0x2012, 0x1000 },
[MICROCHIP_PFSOC_GPIO1] = { 0x20121000, 0x1000 },
[MICROCHIP_PFSOC_GPIO2] = { 0x20122000, 0x1000 },
+[MICROCHIP_PFSOC_RTC] = { 0x20124000, 0x1000 },
[PATCH v3 1/4] hw/riscv: virt: fix uart node name
From: Conor Dooley
"uart" is not a node name that complies with the dt-schema.
Change the node name to "serial" to ix warnings seen during
dt-validate on a dtbdump of the virt machine such as:
/stuff/qemu/qemu.dtb: uart@1000: $nodename:0: 'uart@1000' does not
match '^serial(@.*)?$'
From schema:
/stuff/linux/Documentation/devicetree/bindings/serial/8250.yaml
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: 04331d0b56 ("RISC-V VirtIO Machine")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..6c61a406c4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -917,7 +917,7 @@ static void create_fdt_uart(RISCVVirtState *s, const
MemMapEntry *memmap,
char *name;
MachineState *mc = MACHINE(s);
-name = g_strdup_printf("/soc/uart@%lx", (long)memmap[VIRT_UART0].base);
+name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "ns16550a");
qemu_fdt_setprop_cells(mc->fdt, name, "reg",
--
2.37.1
[PATCH v3 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs
From: Conor Dooley The device trees produced automatically for the virt and spike machines fail dt-validate on several grounds. Some of these need to be fixed in the linux kernel's dt-bindings, but others are caused by bugs in QEMU. I mostly opted for what appeared to be the smallest change that would fix the warnings, partly due to my inexperience with the QEMU codebase. A "sister" patchset for the kernel will clear the remaining warnings. Thanks to Rob Herring for reporting these issues [1], Conor. Changes since v2: - move the syscon subnodes back to the top level instead of into the syscon node Changes since v1: - drop patch 1 To reproduce the errors: ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb (The processed schema needs to be generated first) 0 - https://lore.kernel.org/linux-riscv/20220805162844.1554247-1-m...@conchuod.ie/ 1 - https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/ Conor Dooley (4): hw/riscv: virt: fix uart node name hw/riscv: virt: fix the plic's address cells hw/riscv: virt: fix syscon subnode paths hw/core: fix platform bus node name hw/core/sysbus-fdt.c| 2 +- hw/riscv/virt.c | 8 +--- include/hw/riscv/virt.h | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819 -- 2.37.1
[PATCH v3 3/4] hw/riscv: virt: fix syscon subnode paths
From: Conor Dooley
The reset and poweroff features of the syscon were originally added to
top level, which is a valid path for a syscon subnode. Subsequently a
reorganisation was carried out while implementing NUMA in which the
subnodes were moved into the /soc node. As /soc is a "simple-bus", this
path is invalid, and so dt-validate produces the following warnings:
/stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]],
'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under
{'type': 'object'}
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
/stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]],
'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under
{'type': 'object'}
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
Move the syscon subnodes back to the top level and silence the warnings.
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
Signed-off-by: Conor Dooley
---
I dropped your R-b Alistair intentionally.
Tested both Linux and FreeBSD:
[0.073406] device: 'poweroff': device_add
[0.073441] bus: 'platform': add device poweroff
[0.502347] bus: 'platform': add driver syscon-poweroff
[0.502379] bus: 'platform': __driver_probe_device: matched device poweroff
with driver syscon-poweroff
[0.502397] bus: 'platform': really_probe: probing driver syscon-poweroff
with device poweroff
[0.502423] syscon-poweroff poweroff: no pinctrl handle
[0.502681] syscon-poweroff poweroff: pm_power_off already claimed for
sbi_srst_power_off
[0.50] syscon-poweroff: probe of poweroff failed with error -16
[0.073629] device: 'reboot': device_add
[0.073664] bus: 'platform': add device reboot
[0.500640] bus: 'platform': add driver syscon-reboot
[0.500673] bus: 'platform': __driver_probe_device: matched device reboot
with driver syscon-reboot
[0.500694] bus: 'platform': really_probe: probing driver syscon-reboot with
device reboot
[0.500725] syscon-reboot reboot: no pinctrl handle
[0.502168] driver: 'syscon-reboot': driver_bound: bound to device 'reboot'
[0.502242] bus: 'platform': really_probe: bound device reboot to driver
syscon-reboot
syscon_power0: on ofwbus0
syscon_power1: on ofwbus0
---
hw/riscv/virt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8b2978076e..6f0fd1541b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -896,7 +896,7 @@ static void create_fdt_reset(RISCVVirtState *s, const
MemMapEntry *memmap,
test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
g_free(name);
-name = g_strdup_printf("/soc/reboot");
+name = g_strdup_printf("/reboot");
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
@@ -904,7 +904,7 @@ static void create_fdt_reset(RISCVVirtState *s, const
MemMapEntry *memmap,
qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
g_free(name);
-name = g_strdup_printf("/soc/poweroff");
+name = g_strdup_printf("/poweroff");
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
--
2.37.1
[PATCH v3 2/4] hw/riscv: virt: fix the plic's address cells
From: Conor Dooley
When optional AIA PLIC support was added the to the virt machine, the
address cells property was removed leading the issues with dt-validate
on a dump from the virt machine:
/stuff/qemu/qemu.dtb: plic@c00: '#address-cells' is a required property
From schema:
/stuff/linux/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
Add back the property to suppress the warning.
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: e6faee6585 ("hw/riscv: virt: Add optional AIA APLIC support to virt
machine")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 2 ++
include/hw/riscv/virt.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6c61a406c4..8b2978076e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -465,6 +465,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
qemu_fdt_add_subnode(mc->fdt, plic_name);
qemu_fdt_setprop_cell(mc->fdt, plic_name,
"#interrupt-cells", FDT_PLIC_INT_CELLS);
+qemu_fdt_setprop_cell(mc->fdt, plic_name,
+"#address-cells", FDT_PLIC_ADDR_CELLS);
qemu_fdt_setprop_string_array(mc->fdt, plic_name, "compatible",
(char **)_compat,
ARRAY_SIZE(plic_compat));
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 984e55c77f..be4ab8fe7f 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -111,6 +111,7 @@ enum {
#define FDT_PCI_ADDR_CELLS3
#define FDT_PCI_INT_CELLS 1
+#define FDT_PLIC_ADDR_CELLS 0
#define FDT_PLIC_INT_CELLS1
#define FDT_APLIC_INT_CELLS 2
#define FDT_IMSIC_INT_CELLS 0
--
2.37.1
[PATCH v3 4/4] hw/core: fix platform bus node name
From: Conor Dooley
"platform" is not a valid name for a bus node in dt-schema, so warnings
can be see in dt-validate on a dump of the riscv virt dtb:
/stuff/qemu/qemu.dtb: platform@400: $nodename:0: 'platform@400' does
not match '^([a-z][a-z0-9\\-]+-bus|bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
"platform-bus" is a valid name, so use that instead.
CC: Rob Herring
Fixes: 11d306b9df ("hw/arm/sysbus-fdt: helpers for platform bus nodes addition")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/core/sysbus-fdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index 19d22cbe73..edb0c49b19 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -539,7 +539,7 @@ void platform_bus_add_all_fdt_nodes(void *fdt, const char
*intc, hwaddr addr,
assert(fdt);
-node = g_strdup_printf("/platform@%"PRIx64, addr);
+node = g_strdup_printf("/platform-bus@%"PRIx64, addr);
/* Create a /platform node that we can put all devices into */
qemu_fdt_add_subnode(fdt, node);
--
2.37.1
[PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths
From: Conor Dooley
The subnodes of the syscon have been added to the incorrect paths.
Rather than add them as subnodes, they were originally added to "/foo"
and a later patch moved them to "/soc/foo". Both are incorrect & they
should have been added as "/soc/test@###/foo" as "/soc/test" is the
syscon node. Fix both the reboot and poweroff subnodes to avoid errors
such as:
/stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]],
'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under
{'type': 'object'}
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
/stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]],
'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under
{'type': 'object'}
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8b2978076e..a98b054545 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const
MemMapEntry *memmap,
test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
g_free(name);
-name = g_strdup_printf("/soc/reboot");
+name = g_strdup_printf("/soc/test@%lx/reboot",
+(long)memmap[VIRT_TEST].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
@@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const
MemMapEntry *memmap,
qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
g_free(name);
-name = g_strdup_printf("/soc/poweroff");
+name = g_strdup_printf("/soc/test@%lx/poweroff",
+(long)memmap[VIRT_TEST].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
--
2.37.1
[PATCH v2 1/4] hw/riscv: virt: fix uart node name
From: Conor Dooley
"uart" is not a node name that complies with the dt-schema.
Change the node name to "serial" to ix warnings seen during
dt-validate on a dtbdump of the virt machine such as:
/stuff/qemu/qemu.dtb: uart@1000: $nodename:0: 'uart@1000' does not
match '^serial(@.*)?$'
From schema:
/stuff/linux/Documentation/devicetree/bindings/serial/8250.yaml
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: 04331d0b56 ("RISC-V VirtIO Machine")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..6c61a406c4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -917,7 +917,7 @@ static void create_fdt_uart(RISCVVirtState *s, const
MemMapEntry *memmap,
char *name;
MachineState *mc = MACHINE(s);
-name = g_strdup_printf("/soc/uart@%lx", (long)memmap[VIRT_UART0].base);
+name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "ns16550a");
qemu_fdt_setprop_cells(mc->fdt, name, "reg",
--
2.37.1
[PATCH v2 2/4] hw/riscv: virt: Fix the plic's address cells
From: Conor Dooley
When optional AIA PLIC support was added the to the virt machine, the
address cells property was removed leading the issues with dt-validate
on a dump from the virt machine:
/stuff/qemu/qemu.dtb: plic@c00: '#address-cells' is a required property
From schema:
/stuff/linux/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
Add back the property to suppress the warning.
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: e6faee6585 ("hw/riscv: virt: Add optional AIA APLIC support to virt
machine")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 2 ++
include/hw/riscv/virt.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6c61a406c4..8b2978076e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -465,6 +465,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
qemu_fdt_add_subnode(mc->fdt, plic_name);
qemu_fdt_setprop_cell(mc->fdt, plic_name,
"#interrupt-cells", FDT_PLIC_INT_CELLS);
+qemu_fdt_setprop_cell(mc->fdt, plic_name,
+"#address-cells", FDT_PLIC_ADDR_CELLS);
qemu_fdt_setprop_string_array(mc->fdt, plic_name, "compatible",
(char **)_compat,
ARRAY_SIZE(plic_compat));
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 984e55c77f..be4ab8fe7f 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -111,6 +111,7 @@ enum {
#define FDT_PCI_ADDR_CELLS3
#define FDT_PCI_INT_CELLS 1
+#define FDT_PLIC_ADDR_CELLS 0
#define FDT_PLIC_INT_CELLS1
#define FDT_APLIC_INT_CELLS 2
#define FDT_IMSIC_INT_CELLS 0
--
2.37.1
[PATCH v2 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs
From: Conor Dooley The device trees produced automatically for the virt and spike machines fail dt-validate on several grounds. Some of these need to be fixed in the linux kernel's dt-bindings, but others are caused by bugs in QEMU. I mostly opted for what appeared to be the smallest change that would fix the warnings, partly due to my inexperience with the QEMU codebase. A "sister" patchset for the kernel will clear the remaining warnings [0]. Thanks to Rob Herring for reporting these issues [1], Conor. Changes since v1: - drop patch 1 To reproduce the errors: ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb (The processed schema needs to be generated first) 0 - https://lore.kernel.org/linux-riscv/20220805162844.1554247-1-m...@conchuod.ie 1 - https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org Conor Dooley (4): hw/riscv: virt: fix uart node name hw/riscv: virt: Fix the plic's address cells hw/riscv: virt: fix syscon subnode paths hw/core: fix platform bus node name hw/core/sysbus-fdt.c| 2 +- hw/riscv/virt.c | 10 +++--- include/hw/riscv/virt.h | 1 + 3 files changed, 9 insertions(+), 4 deletions(-) base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819 -- 2.37.1
[PATCH v2 4/4] hw/core: fix platform bus node name
From: Conor Dooley
"platform" is not a valid name for a bus node in dt-schema, so warnings
can be see in dt-validate on a dump of the riscv virt dtb:
/stuff/qemu/qemu.dtb: platform@400: $nodename:0: 'platform@400' does
not match '^([a-z][a-z0-9\\-]+-bus|bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
"platform-bus" is a valid name, so use that instead.
CC: Rob Herring
Fixes: 11d306b9df ("hw/arm/sysbus-fdt: helpers for platform bus nodes addition")
Reviewed-by: Alistair Francis
Signed-off-by: Conor Dooley
---
hw/core/sysbus-fdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index 19d22cbe73..edb0c49b19 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -539,7 +539,7 @@ void platform_bus_add_all_fdt_nodes(void *fdt, const char
*intc, hwaddr addr,
assert(fdt);
-node = g_strdup_printf("/platform@%"PRIx64, addr);
+node = g_strdup_printf("/platform-bus@%"PRIx64, addr);
/* Create a /platform node that we can put all devices into */
qemu_fdt_add_subnode(fdt, node);
--
2.37.1
[PATCH 3/5] hw/riscv: virt: Fix the plic's address cells
From: Conor Dooley
When optional AIA PLIC support was added the to the virt machine, the
address cells property was removed leading the issues with dt-validate
on a dump from the virt machine:
/stuff/qemu/qemu.dtb: plic@c00: '#address-cells' is a required property
From schema:
/stuff/linux/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
Add back the property to suppress the warning.
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: e6faee6585 ("hw/riscv: virt: Add optional AIA APLIC support to virt
machine")
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 2 ++
include/hw/riscv/virt.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6c61a406c4..8b2978076e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -465,6 +465,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
qemu_fdt_add_subnode(mc->fdt, plic_name);
qemu_fdt_setprop_cell(mc->fdt, plic_name,
"#interrupt-cells", FDT_PLIC_INT_CELLS);
+qemu_fdt_setprop_cell(mc->fdt, plic_name,
+"#address-cells", FDT_PLIC_ADDR_CELLS);
qemu_fdt_setprop_string_array(mc->fdt, plic_name, "compatible",
(char **)_compat,
ARRAY_SIZE(plic_compat));
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 984e55c77f..be4ab8fe7f 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -111,6 +111,7 @@ enum {
#define FDT_PCI_ADDR_CELLS3
#define FDT_PCI_INT_CELLS 1
+#define FDT_PLIC_ADDR_CELLS 0
#define FDT_PLIC_INT_CELLS1
#define FDT_APLIC_INT_CELLS 2
#define FDT_IMSIC_INT_CELLS 0
--
2.37.1
[PATCH 4/5] hw/riscv: virt: fix syscon subnode paths
From: Conor Dooley
The subnodes of the syscon have been added to the incorrect paths.
Rather than add them as subnodes, they were originally added to "/foo"
and a later patch moved them to "/soc/foo". Both are incorrect & they
should have been added as "/soc/test@###/foo" as "/soc/test" is the
syscon node. Fix both the reboot and poweroff subnodes to avoid errors
such as:
/stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]],
'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under
{'type': 'object'}
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
/stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]],
'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under
{'type': 'object'}
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8b2978076e..a98b054545 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const
MemMapEntry *memmap,
test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
g_free(name);
-name = g_strdup_printf("/soc/reboot");
+name = g_strdup_printf("/soc/test@%lx/reboot",
+(long)memmap[VIRT_TEST].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
@@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const
MemMapEntry *memmap,
qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
g_free(name);
-name = g_strdup_printf("/soc/poweroff");
+name = g_strdup_printf("/soc/test@%lx/poweroff",
+(long)memmap[VIRT_TEST].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
--
2.37.1
[PATCH 1/5] target/riscv: Ignore the S and U letters when formatting ISA strings
From: Palmer Dabbelt
The ISA strings we're providing from QEMU aren't actually legal RISC-V
ISA strings, as both S and U cannot exist as single-letter extensions
and must instead be multi-letter strings. We're still using the ISA
strings inside QEMU to track the availiable extensions, so just strip
out the S and U extensions when formatting ISA strings.
Signed-off-by: Palmer Dabbelt
[Conor: rebased on 7.1.0-rc1 & slightly tweaked the commit message]
Signed-off-by: Conor Dooley
---
target/riscv/cpu.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ac6f82ebd0..95fdc03b3d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1122,7 +1122,23 @@ char *riscv_isa_string(RISCVCPU *cpu)
char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
for (i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
-*p++ = qemu_tolower(riscv_single_letter_exts[i]);
+char lower = qemu_tolower(riscv_single_letter_exts[i]);
+switch (lower) {
+case 's':
+case 'u':
+/*
+* The 's' and 'u' letters shouldn't show up in ISA strings
as
+* they're not extensions, but they should show up in MISA.
+* Since we use these letters interally as a pseudo ISA
string
+* to set MISA it's easier to just strip them out when
+* formatting the ISA string.
+*/
+break;
+
+default:
+*p++ = lower;
+break;
+}
}
}
*p = '\0';
--
2.37.1
[PATCH 5/5] hw/core: fix platform bus node name
From: Conor Dooley
"platform" is not a valid name for a bus node in dt-schema, so warnings
can be see in dt-validate on a dump of the riscv virt dtb:
/stuff/qemu/qemu.dtb: platform@400: $nodename:0: 'platform@400' does
not match '^([a-z][a-z0-9\\-]+-bus|bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
From schema:
/home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
"platform-bus" is a valid name, so use that instead.
CC: Rob Herring
Fixes: 11d306b9df ("hw/arm/sysbus-fdt: helpers for platform bus nodes addition")
Signed-off-by: Conor Dooley
---
hw/core/sysbus-fdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index 19d22cbe73..edb0c49b19 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -539,7 +539,7 @@ void platform_bus_add_all_fdt_nodes(void *fdt, const char
*intc, hwaddr addr,
assert(fdt);
-node = g_strdup_printf("/platform@%"PRIx64, addr);
+node = g_strdup_printf("/platform-bus@%"PRIx64, addr);
/* Create a /platform node that we can put all devices into */
qemu_fdt_add_subnode(fdt, node);
--
2.37.1
[PATCH 2/5] hw/riscv: virt: fix uart node name
From: Conor Dooley
"uart" is not a node name that complies with the dt-schema.
Change the node name to "serial" to ix warnings seen during
dt-validate on a dtbdump of the virt machine such as:
/stuff/qemu/qemu.dtb: uart@1000: $nodename:0: 'uart@1000' does not
match '^serial(@.*)?$'
From schema:
/stuff/linux/Documentation/devicetree/bindings/serial/8250.yaml
Reported-by: Rob Herring
Link:
https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
Fixes: 04331d0b56 ("RISC-V VirtIO Machine")
Signed-off-by: Conor Dooley
---
hw/riscv/virt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..6c61a406c4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -917,7 +917,7 @@ static void create_fdt_uart(RISCVVirtState *s, const
MemMapEntry *memmap,
char *name;
MachineState *mc = MACHINE(s);
-name = g_strdup_printf("/soc/uart@%lx", (long)memmap[VIRT_UART0].base);
+name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
qemu_fdt_add_subnode(mc->fdt, name);
qemu_fdt_setprop_string(mc->fdt, name, "compatible", "ns16550a");
qemu_fdt_setprop_cells(mc->fdt, name, "reg",
--
2.37.1
[PATCH 0/5] QEMU: Fix RISC-V virt & spike machines' dtbs
From: Conor Dooley The device trees produced automatically for the virt and spike machines fail dt-validate on several grounds. Some of these need to be fixed in the linux kernel's dt-bindings, but others are caused by bugs in QEMU. Patch one of this series is lifted from an earlier submission by Palmer that seems to have got lost by the wayside somewhere along the way, hence the rather out of date email used for Palmer [0]. I mostly opted for what appeared to be the smallest change that would fix the warnings, partly due to my inexperience with the QEMU codebase. A "sister" patchset for the kernel will clear the remaining warnings. Thanks to Rob for reporting these issues [1], Conor. To reproduce the errors: ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb (The processed schema needs to be generated first) 0 - https://lore.kernel.org/qemu-devel/20190813225307.5792-1-pal...@sifive.com/ 1 - https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/ Conor Dooley (4): hw/riscv: virt: fix uart node name hw/riscv: virt: Fix the plic's address cells hw/riscv: virt: fix syscon subnode paths hw/core: fix platform bus node name Palmer Dabbelt (1): target/riscv: Ignore the S and U letters when formatting ISA strings hw/core/sysbus-fdt.c| 2 +- hw/riscv/virt.c | 10 +++--- include/hw/riscv/virt.h | 1 + target/riscv/cpu.c | 18 +- 4 files changed, 26 insertions(+), 5 deletions(-) base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819 -- 2.37.1
