On Tue, Dec 11, 2018 at 02:40:07PM +0100, Christoffer Dall wrote:
> On Tue, Dec 11, 2018 at 01:11:33PM +0000, Andrew Murray wrote:
> > On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> > > On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > > > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > > > type, at present this is typedef'd to kvm_cpu_context and used to store
> > > > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > > > hold vcpu context. In order to use the percpu to hold additional future
> > > > host information we encapsulate kvm_cpu_context in a new structure.
> > > >
> > > > Signed-off-by: Andrew Murray <andrew.murray@xxxxxxx>
> > > > ---
> > > > arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> > > > arch/arm64/kernel/asm-offsets.c | 3 ++-
> > > > virt/kvm/arm/arm.c | 4 +++-
> > > > 3 files changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index 1550192..bcf9d60 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> > > > struct kvm_vcpu *__hyp_running_vcpu;
> > > > };
> > > >
> > > > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > +struct kvm_host_data {
> > > > + struct kvm_cpu_context __kvm_cpu_state;
> > > > +};
> > > > +
> > > > +typedef struct kvm_host_data kvm_cpu_context_t;
> > >
> > > Now I'm confused based on the conversation on the last version.
> > >
> > > I think it's bizarre to use the typedef to rename things in this way.
> > >
> > > Can you please make this:
> > >
> > > struct kvm_cpu_context;
> > > typedef struct kvm_cpu_context kvm_cpu_context_t;
> > >
> > > struct kvm_host_data;
> > > typedef struct kvm_host_data kvm_host_data_t;
> > >
> > > And change the code with the fallout from that.
> >
> > I guess I was trying to avoid similar naming issues on arm32. If we
> > make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
> > then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:
> >
> > typedef struct kvm_cpu_context kvm_cpu_context_t;
> >
> > becomes:
> >
> > typedef struct kvm_cpu_context kvm_host_data_t;
> >
> > though I guess this may be acceptable?
>
> I'd prefer it if you just introduce a struct kvm_host_data on the 32-bit
> side only containing a struct kvm_cpu_context (if you wanted to support
> perf on the 32-bit side you would also add additional fields to it,
> similar to arm64). That avoids the confusing typedef and you get the
> symmmetry on both architectures allowing you to use shared code, which
> is what we want at the end of the day.
>
> So, on the 32-bit side, change this to:
>
> struct kvm_host_data {
> struct kvm_cpu_context *host_ctxt;
> };
> typedef struct kvm_host_data kvm_host_data_t;
>
>
> And use the same naming on both 32-bit and 64-bit arm, consistently.
Sounds good to me.
Thanks,
Andrew Murray
>
>
> Thanks,
>
> Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
