Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FMV] Explicit allows different vendors share the bit definition of __riscv_vendor_feature_bits #96

Open
cyyself opened this issue Nov 14, 2024 · 5 comments

Comments

@cyyself
Copy link
Contributor

cyyself commented Nov 14, 2024

In PR #74, we design vendor extension is guarded by vendorID. However, here we missed some cases like:

  1. A SoC vendor buys a CPU IP from another CPU IP vendor with some modification to CPU RTL and then fills marchid with itself mvendorid
  2. A SoC vendor uses multiple CPU IPs from different vendors for their SoC and customizes the CPU IP to have the same vendor extension, which multiple CPU IPs have different mvendorid

These cases are discussed from Linux Kernel mailing list on hwprobe support for T-HEAD vendor extensions thread. I think their concerns are right.

So we might need to make the __riscv_vendor_feature_bits not guarded by mvendorid. Since this hasn't been specified in the c-api-doc currently, it's not too late to make this change.

@cyyself
Copy link
Contributor Author

cyyself commented Nov 14, 2024

cc @kito-cheng @BeMg @ConchuOD @charlie-rivos @evangreen

@cyyself cyyself changed the title [FMV] Expilicit allows different vendor share the bit defination of __riscv_vendor_feature_bits [FMV] Explicit allows different vendor share the bit definition of __riscv_vendor_feature_bits Nov 14, 2024
@cyyself cyyself changed the title [FMV] Explicit allows different vendor share the bit definition of __riscv_vendor_feature_bits [FMV] Explicit allows different vendors share the bit definition of __riscv_vendor_feature_bits Nov 14, 2024
@jrtc27
Copy link

jrtc27 commented Jan 16, 2025

I agree the current interface is insufficient. One option would be to have __riscv_vendor_feature_bits be a list of (vendor, pointer) pairs, where the pointer points to an individual instance of what __riscv_vendor_feature_bits is today (i.e. the per-vendor bits). But for now we should just remove __riscv_vendor_feature_bits from the spec now and come up with a fixed solution in future.

@kito-cheng
Copy link
Collaborator

Yeah, I agree to remove that for now since we don't have any vendor bits yet. so maybe we postpone that a little bit is best way.

@cyyself could you create a PR to drop that? and then we could have few more time to discuss the mechanism.

@cyyself
Copy link
Contributor Author

cyyself commented Jan 16, 2025

Yeah, I agree to remove that for now since we don't have any vendor bits yet. so maybe we postpone that a little bit is best way.

@cyyself could you create a PR to drop that? and then we could have few more time to discuss the mechanism.

OK. But it’s too late at UTC+8. Maybe I will do that tomorrow.

cyyself added a commit to cyyself/riscv-c-api-doc that referenced this issue Jan 17, 2025
As discussed in riscv-non-isa#96, current interface is insufficient to support some
cases, like a vendor buying a CPU IP from the upstream vendor but using
their own mvendorid and custom features from the upstream vendor. In
this case, we might need to add these extensions for each downstream
vendor many times. Thus, making __riscv_vendor_feature_bits guarded by
mvendorid is not a good idea. So, drop __riscv_vendor_feature_bits for
now, and we should have time to discuss a better solution.
@cyyself
Copy link
Contributor Author

cyyself commented Jan 17, 2025

One option would be to have __riscv_vendor_feature_bits be a list of (vendor, pointer) pairs, where the pointer points to an individual instance of what __riscv_vendor_feature_bits is today (i.e. the per-vendor bits).

A minor concern is that we might need a mechanism to allocate memory for each individual instance for each vendor extension set in an IFUNC resolver, which will be called from __init_riscv_cpu_feature_bits. A simple solution might be to allocate a fixed number of these objects, like 4 or 8, which is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants