Hi Christoffer,
Thanks for the review! I've rebased, and am applying your
feedback now.
On Thu, 2 Aug 2012 19:11:22 +0200, Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jul 12, 2012 at 7:54 AM, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> > @@ -374,6 +485,41 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > return emulate_cp15(vcpu, ¶ms);
> > }
> >
> > +static void reset_table(struct kvm_vcpu *vcpu,
> > + const struct coproc_reg *table, size_t num)
>
> nit: this function name seems to imply that some table is being reset,
> which is a bit of a stretch (I guess you can see the array in the
> vcpu->arch struct as sort of a table, but... hmmm... maybe just fold
> it into the function below is fine?
Yes, the name is lazy. Let's just call it reset_coproc_regs(). Feel
free to re-rename!
> > + * virtual CPU struct to their architectually defined reset values.
>
> s/architectually/architecturally/
That's embarrassing. Fixed.
> hmm, I'm not sure I agree with this all together. The only benefit I
> can see is that the coprocessor information is kept in a single place,
> but it comes at the cost of complicating the emulation table and makes
> it less obvious where we needed explicit emulation code.
>
> Bring it on ;)
Agreed, I toyed with using a separate table for non-emulated regs, but
we already have generic and arch-specific tables, and a further split
seemed like too many.
I think with the MSR-style API, it's a net win. But hence the move from
emulate.c to coproc.c, since it's now a generic table.
> On Thu, Jul 12, 2012 at 7:54 AM, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> > + /* Initialization for vcpu. */
> > + void (*reset)(struct kvm_vcpu *, const struct coproc_reg *);
> > +
> > + /* Offset of register in vcpu->arch.cp15[], or 0. */
>
> 0 means hide from user space or not backed by vcpu->arch entry for
> whatever other reason, right? perhaps worth including in the comment,
> the "or 0" is not very helpful
OK, I thought about this some more. How's this?
/* Index into vcpu->arch.cp15[], or 0 if we don't need to save it. */
enum cp15_regs reg;
I didn't want to get into why we don't need to save it; it's not just
that it's R/O, but also that qemu can't set it.
> > @@ -243,6 +292,33 @@ static bool pm_fake(struct kvm_vcpu *vcpu,
> >
> > /* Architected CP15 registers. */
> > static const struct coproc_reg cp15_regs[] = {
> > + /* TTBCR: swapped by interrupt.S. */
> > + { CRn( 2), CRm( 0), Op1( 0), Op2( 0), is32,
> > + NULL, reset_val, c2_TTBCR, 0x00000000 },
>
> why the very long version of 0 ?
Very minor preference: I tried to differentiate from a random 0 which
would occur without an initializer. This one is needed for reset_val().
> > +
> > + /* TTBR0/TTBR1: swapped by interrupt.S. */
Cheers,
Rusty.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
