On Mon, 15 Apr 2019 16:56:08 -0700
Alistair Francis <alistai...@gmail.com> wrote:
> 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
yes, it's clunky and verbose but it's uniform for all cpus,
which is doubly important for machine users and other interface QEMU
has to communicate with users.
secondly, adding set of symbols to cpu name (ideally it should be cpu's
typename)
complicates parsing and adds one more user to the old compat hooks, which
prevents us from deprecating those.
We've put a lot of of effort in consolidating these parts across QEMU,
so adding yet another custom way to parse -cpu looks like regression to me.
> > 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.
I'd go for this one as it's less controversial and follows common patterns.
Human users can write a wrapper script if necessary.
>
> Alistair
> ...
