On Mon, Apr 15, 2019 at 1:38 AM Igor Mammedov <imamm...@redhat.com> wrote:
>
> On Fri, 12 Apr 2019 14:19:16 -0700
> Alistair Francis <alistai...@gmail.com> wrote:
>
...
> > >
> > > if it's programming error it should be assert
> > > but in general instance_init() should never fail.
> >
> > We are checking for a user specified CPU string, not a programming
> > error so an assert() isn't correct here.
> >
> > If *init() can't fail how should this be handled?
> typical flow for device object is
>
> object_new(foo) -> foo_init_fn()
> set properties on foo
> set 'realize' property to 'true' -> foo_realize_fn()
>
> all checks should be performed in realize() or during property setting.
I thought there was a reason realise didn't work, but now I can't see
what it is. I can move it to realise.
>
> > > the same applies to errors or warnings within this function.
> > >
> > > > > > + }
> > > > > > +
> > > > > > + if (riscv_cpu[0] == 'r' && riscv_cpu[1] == 'v') {
> > > > > > + /* Starts with "rv" */
> > > > > > + if (riscv_cpu[2] == '3' && riscv_cpu[3] == '2') {
> > > > > > + valid = true;
> > > > > > + rvxlen = RV32;
> > > > > > + }
> > > > > > + if (riscv_cpu[2] == '6' && riscv_cpu[3] == '4') {
> > > > > > + valid = true;
> > > > > > + rvxlen = RV64;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + if (!valid) {
> > > > > > + error_report("'%s' does not appear to be a valid RISC-V
> > > > > > CPU",
> > > > > > + riscv_cpu);
> > > > > > + exit(1);
> > > > > > + }
> > > > > > +
> > > > > > + for (i = 4; i < strlen(riscv_cpu); i++) {
> > > > > > + switch (riscv_cpu[i]) {
> > > > > > + case 'i':
> > > > > > + if (target_misa & RVE) {
> > > > > > + error_report("I and E extensions are
> > > > > > incompatible");
> > > > > > + exit(1);
> > > > > > + }
> > > > > > + target_misa |= RVI;
> > > > > > + continue;
> > > > > > + case 'e':
> > > > > > + if (target_misa & RVI) {
> > > > > > + error_report("I and E extensions are
> > > > > > incompatible");
> > > > > > + exit(1);
> > > > > > + }
> > > > > > + target_misa |= RVE;
> > > > > > + continue;
> > > > > > + case 'g':
> > > > > > + target_misa |= RVI | RVM | RVA | RVF | RVD;
> > > > > > + continue;
> > > > > > + case 'm':
> > > > > > + target_misa |= RVM;
> > > > > > + continue;
> > > > > > + case 'a':
> > > > > > + target_misa |= RVA;
> > > > > > + continue;
> > > > > > + case 'f':
> > > > > > + target_misa |= RVF;
> > > > > > + continue;
> > > > > > + case 'd':
> > > > > > + target_misa |= RVD;
> > > > > > + continue;
> > > > > > + case 'c':
> > > > > > + target_misa |= RVC;
> > > > > > + continue;
> > > > > > + case 's':
> > > > > > + target_misa |= RVS;
> > > > > > + continue;
> > > > > > + case 'u':
> > > > > > + target_misa |= RVU;
> > > > > > + continue;
> > > > > > + default:
> > > > > > + warn_report("QEMU does not support the %c extension",
> > > > > > + riscv_cpu[i]);
> > > that's what looks to me like fallback, and what makes
> > > CPU features for this CPU type non deterministic.
> >
> > What do you mean? QEMU will always parse the specified CPU string and
> > create a CPU based on the features specified by the user. For each
> > version of QEMU it will always result in the same outcome for the same
> > user supplied command line argument.
>
> I've meant that here you would print warning and happily continue parsing
> user input ignoring input error. One should error out instead and make
> user fix error.
Ah, ok. I can change this one to an error.
>
> >
> > Newer QEMU versions will support more features though, so they will
> > accept different options.
> >
> > > I'm not sure what you are trying to achieve here, but it look like
> > > you are trying to verify MachineClass field in object constructor
> > > along with other things.
> >
> > I'm just trying to parse the -cpu string option.
> >
> > >
> > > I'd put all MachineClass checks in class_init and abort on error
> > > since invalid mcc->isa_str is codding error.
> >
> > No, it's a user error.
>
> Parsing -cpu by assigning to class members user input looks unusual.
>
> We had a custom parsing for x86 & sparc cpus, but that was factored out
> into compat utilities and we shouldn't do it anywhere else without very
> compelling reason.
I did see that, but it didn't seem to work the same way as this.
>
> Currently it's preferred to use canonical form for cpu properties, like:
>
> -cpu foo,prop1=x,prop2=y,...
Yeah, but I don't see how that would work nicely.
Instead of:
-cpu rv64gcsu
we would have:
-cpu rvgen,xlen=64,g=true,c=true,s=true,u=true
which seems more complex for users
>
> and let the generic parser to do the job. For it to work you would need
> to create a property per a cpu feature.
>
> This way it will work fine with -cpu and -device/device_add without any
> custom parsing involved.
Although I think it's messy for users, it is the cleanest
implementation in terms of QOM. Maybe that will be the only solution.
Alistair
...
