Re: [PATCH 4/5] KVM: ARM: Change instruction decoding to use struct kvm_decode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 6 Nov 2012 00:37:10 +0100, Christoffer Dall
<c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Nov 5, 2012 at 11:28 AM, Marc Zyngier <marc.zyngier@xxxxxxx>
wrote:
>> On 04/11/12 22:59, Christoffer Dall wrote:
>>> Prepare to factor out load/store instruction decoding by using struct
>>> kvm_decode and operate on a separate copy of struct pt_regs.
>>
>> I quite like the approach, except for a couple of points below.
>>
>>> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>>  arch/arm/include/asm/kvm_host.h |   12 ++++--
>>>  arch/arm/kvm/emulate.c          |   84
>>>  ++++++++++++++++++++++++---------------
>>>  arch/arm/kvm/mmio.c             |   22 +++++-----
>>>  3 files changed, 70 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_host.h
>>> b/arch/arm/include/asm/kvm_host.h
>>> index 6c1c6fc..439deb0 100644
>>> --- a/arch/arm/include/asm/kvm_host.h
>>> +++ b/arch/arm/include/asm/kvm_host.h
>>> @@ -78,6 +78,13 @@ struct kvm_mmu_memory_cache {
>>>         void *objects[KVM_NR_MEM_OBJS];
>>>  };
>>>
>>> +struct kvm_decode {
>>> +       struct pt_regs *regs;
>>> +       unsigned long hxfar;
>>
>> Can we call this field "far" instead? We don't need to know it codes
>> from the hypervisor, and it will always be data.
>>
> 
> yes
> 
>>> +       unsigned long rt;       /* destination register for loads */
>>> +       bool sign_extend;       /* for byte/halfword loads */
>>> +};
>>> +
>>>  struct kvm_vcpu_arch {
>>>         struct kvm_regs regs;
>>>
>>> @@ -115,10 +122,7 @@ struct kvm_vcpu_arch {
>>>         bool pause;
>>>
>>>         /* IO related fields */
>>> -       struct {
>>> -               bool sign_extend;       /* for byte/halfword loads */
>>> -               u32  rd;
>>> -       } mmio;
>>> +       struct kvm_decode mmio_decode;
>>>
>>>         /* Interrupt related fields */
>>>         u32 irq_lines;          /* IRQ and FIQ levels */
>>> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
>>> index ca72be9..7cbdc68 100644
>>> --- a/arch/arm/kvm/emulate.c
>>> +++ b/arch/arm/kvm/emulate.c
>>> @@ -304,7 +304,7 @@ struct arm_instr {
>>>         bool sign_extend;
>>>         bool w;
>>>
>>> -       bool (*decode)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio
>>> *mmio,
>>> +       bool (*decode)(struct kvm_decode *decode, struct kvm_exit_mmio
>>> *mmio,
>>>                        unsigned long instr, struct arm_instr *ai);
>>>  };
>>>
>>> @@ -380,7 +380,7 @@ u32 shift(u32 value, u8 N, enum SRType type, u8
>>> amount, bool carry_in)
>>>         return value & mask;
>>>  }
>>>
>>> -static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio
>>> *mmio,
>>> +static bool decode_arm_wb(struct kvm_decode *decode, struct
>>> kvm_exit_mmio *mmio,
>>>                           unsigned long instr, const struct arm_instr
>>>                           *ai)
>>>  {
>>>         u8 Rt = (instr >> 12) & 0xf;
>>> @@ -388,7 +388,7 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu,
>>> struct kvm_exit_mmio *mmio,
>>>         u8 W = (instr >> 21) & 1;
>>>         u8 U = (instr >> 23) & 1;
>>>         u8 P = (instr >> 24) & 1;
>>> -       u32 base_addr = *vcpu_reg(vcpu, Rn);
>>> +       u32 base_addr = decode->regs->uregs[Rn];
>>
>> Can we please have accessors in some include file? At lease for stuff
>> that is architecture specific? Something like:
>>
>> static inline unsigned long *kvm_decode_reg(struct kvm_decode *decode,
>>                                             int reg)
>> {
>>         return &decode->regs->uregs[reg];
>> }
>>
>> so it is broadly equivalent to vcpu_reg? This would save me a lot of
>> hassle for arm64.
>>
> 
> sure
> 
>>>         u32 offset_addr, offset;
>>>
>>>         /*
>>> @@ -403,14 +403,14 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu,
>>> struct kvm_exit_mmio *mmio,
>>>                 return false;
>>>         }
>>>
>>> -       vcpu->arch.mmio.rd = Rt;
>>> +       decode->rt = Rt;
>>>
>>>         if (ai->register_form) {
>>>                 /* Register operation */
>>>                 enum SRType s_type;
>>>                 u8 shift_n;
>>> -               bool c_bit = *vcpu_cpsr(vcpu) & PSR_C_BIT;
>>> -               u32 s_reg = *vcpu_reg(vcpu, ai->Rm);
>>> +               bool c_bit = decode->regs->ARM_cpsr & PSR_C_BIT;
>>
>> static inline unsigned long kvm_decode_cpsr(struct kvm_decode *decode)
>> {
>>         return decode->regs->ARM_cpsr;
>> }
>>
> 
> if this will make things easier for arm64, sure, otherwise I'm not
> sure I agree this improves readability. (I though you weren't going to
> support this type of stuff for arm64?)

I have no plan to support this initially on arm64, as this would another
level of complexity we don't really need at this stage. But I'm convinced
someone will eventually try to plug this in at one point, and I'd like the
interface to be "good enough" from the beginning.

        M.
-- 
Fast, cheap, reliable. Pick two.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm


[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux