On 01/09/17 10:44, Christoffer Dall wrote:
> On Fri, Sep 01, 2017 at 12:10:59PM +0800, wanghaibin wrote:
>> On 2017/9/1 4:33, Christoffer Dall wrote:
>>
>>> Hi Wanghaibin,
>>>
>>> On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
>>>> v3: Coding style fix.
>>>> Add the valid APRn access check which Marc proposed.
>>>>
>>>> v2: Split the patch again to make it easier for review
>>>> some fixes were proposed by Marc
>>>
>>> Usually we put the changelog at the end of the description, before the
>>> diff state. I suggest you have a look at other patch series on the
>>> kvmarm list or on the ARM linux mailing list and see how most people do
>>> it.
>>
>>
>> OK, Pay attention next time.
>>
>> Thanks.
>>
>>>
>>>>
>>>> v1: the problem describe:
>>>> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
>>>> vm boot => migrate to destination => shutdown at destination => start at destination
>>>> => migrate back to source ... go round and begin again;
>>>>
>>>> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm
>>>> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
>>>> while failed, we can watch the interrupts in the vm, some vcpus of the vm can not
>>>> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
>>>>
>>>> vgic_state debug file at destination shows(a active virt timer interrupt) that:
>>>> VCPU 0 TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID
>>>> ---------------------------------------------------------------
>>>> ....................
>>>> PPI 25 0 000001 0 1 0 160 -1
>>>> PPI 26 0 000001 0 1 0 160 -1
>>>> PPI 27 0 011111 27 1 0 160 0
>>>>
>>>>
>>>> I added log to print ICH* registers for VCPU0 at destination:
>>>> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
>>>> -----AP0R:0: 0x0------
>>>> -----AP0R:1: 0x0------
>>>> -----AP0R:2: 0x0------
>>>> -----AP0R:3: 0x0------
>>>> -----AP1R:0: 0x0------
>>>> -----AP1R:1: 0x0------
>>>> -----AP1R:2: 0x0------
>>>> -----AP1R:3: 0x0------
>>>> -----LR:0: 0xa0a0001b0000001b------
>>>> -----LR:1: 0x0------
>>>> -----LR:2: 0x0------
>>>> -----LR:3: 0x0------
>>>>
>>>> and the ICH_AP1R0 value is 0x10000 at source.
>>>>
>>>> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
>>>> and kvm does not support the uaccess interface. This patchset try to support this.
>>>>
>>>
>>> So I feel like this series is horribly complicated for what it does, and
>>> does things in the reverse order. If you really want to take your
>>> appraoch, it would be much nicer if you first changed existing functions
>>> and added infrastructure, and then in the end wired it up to the user
>>> space ABI. In other words, reverse your patches.
>>
>>
>> No problem. The patch order can be adjusted if you feel necessary (this
>> depends on the results of the simpler patch discussion).
>>
>>>
>>> However, I think I have a simpler approach to solving this. Please have
>>> a look at this:
>>>
>>> commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
>>> Author: Christoffer Dall <cdall@xxxxxxxxxx>
>>> Date: Thu Aug 31 22:24:25 2017 +0200
>>>
>>> KVM: arm/arm64: Support uaccess of GICC_APRn
>>>
>>> When migrating guests around we need to know the active priorities to
>>> ensure functional virtual interrupt prioritization by the GIC.
>>>
>>> This commit clarifies the API and how active priorities of interrupts in
>>> different groups are represented, and implements the accessor functions
>>> for the uaccess register range.
>>>
>>> We live with a slight layering violation in accessing GICv3 data
>>> structures from vgic-mmio-v2.c, because anything else just adds too much
>>> complexity for us to deal with (it's not like there's a benefit
>>> elsewhere in the code of an intermediate representation as is the case
>>> with the VMCR). We accept this, because while doing v3 processing from
>>> a file named something-v2.c can look strange at first, this really is
>>> specific to dealing with the user space interface for something that
>>> looks like a GICv2.
>>>
>>
>>
>> I have different opinions here.
>>
>> Form Marc's proposed, I guess we take the vgic-v2.c/ vgic-v3.c as the hardware abstraction
>> layer for GICH_* / ICH_* registers. These files provide a series of methods to interactive with
>> registers (not only vgic_vmcr, such as vgic_hcr, vgic_elrsr, vgic_lr), these registers only can
>> be changed by the provided methods (eq: vgic_v2/v3_fold/populate_lr_state, vgic_v2/v3_set_underflow)
>> and finally, these methods are invoking by the uniform interface (vgic_set_underflow,
>> vgic_populate/fold_lr, these uniform interfaces invoked the corresponding method through HW probed info)
>>
>> In a word, I think we should design the high cohesion and low coupling code, and we've been doing this too.
>> We should strictly restrict other modules or files to know or access the vgic_v2/v3_cpu_if or
>> its registers (Unfortunately, vgic_sre and vgic_aprn are breaking the rules currently, the patch 4 fix
>> vgicv3 sys access, and this patchset design followed this rule).
>>
>> My understanding is as mentioned above, maybe not correct.
>>
>
> There are no strict layering definitions, exported ABIs or even APIs
> here. We split things into multiple files to make the code readable and
> easy to maintain and understand.
>
> Comparing the diff state (6 files, 126 inserts, 22 deletes in your
> version, 2 files, 45 inserts, 1 delete in my version) speaks to some of
> the completexity of the two approaches.
>
> The biggest problem is that it takes me hours to understand your patch
> series, which indicates that it's over-designed or fails to convery the
> necessity of the complication.
>
>>
>>
>>
>> Put aside the discussion of the abstract layer, same to Marc's proposed:
>> Avoid to allow userspace to save/restore undefined APR register, that doesn't
>> make sense.
>>
>
> I don't see it as a big problem; they will have RAZ/WI semantics towards
> the VM, similar to hardware. The only downside is that you can write
> something into registers that are not used to the VM and read back that
> value, when accessing from userspace. However, if we really want to do
> this, I'd argue this is much simpler:
>
> commit 5cd35287d086c99859900a3d7630a12c9d549fad
> Author: Christoffer Dall <cdall@xxxxxxxxxx>
> Date: Fri Sep 1 11:41:52 2017 +0200
>
> KVM: arm/arm64: Extract GICv3 max APRn index calculation
>
> As we are about to access the APRs from the GICv2 uaccess interface,
> make this logic generally available.
>
> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx>
>
> diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
> index 116786d..c77d508 100644
> --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
> +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
> @@ -208,29 +208,12 @@ static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
> static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> const struct sys_reg_desc *r, u8 apr)
> {
> - struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
> u8 idx = r->Op2 & 3;
>
> - /*
> - * num_pri_bits are initialized with HW supported values.
> - * We can rely safely on num_pri_bits even if VM has not
> - * restored ICC_CTLR_EL1 before restoring APnR registers.
> - */
> - switch (vgic_v3_cpu->num_pri_bits) {
> - case 7:
> - vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> - break;
> - case 6:
> - if (idx > 1)
> - goto err;
> - vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> - break;
> - default:
> - if (idx > 0)
> - goto err;
> - vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> - }
> + if (idx > vgic_v3_max_apr_idx(vcpu))
> + goto err;
>
> + vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> return true;
> err:
> if (!p->is_write)
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index bba7fa2..bf9ceab 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -220,4 +220,20 @@ int vgic_debug_destroy(struct kvm *kvm);
> bool lock_all_vcpus(struct kvm *kvm);
> void unlock_all_vcpus(struct kvm *kvm);
>
> +static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
> +{
> + struct vgic_cpu *cpu_if = &vcpu->arch.vgic_cpu;
> +
> + /*
> + * num_pri_bits are initialized with HW supported values.
> + * We can rely safely on num_pri_bits even if VM has not
> + * restored ICC_CTLR_EL1 before restoring APnR registers.
> + */
> + switch (cpu_if->num_pri_bits) {
> + case 7: return 3;
> + case 6: return 1;
> + default: return 0;
> + }
> +}
> +
> #endif
>
>
>
> commit 660efe48259ebe83e6f93cba0a184327cebf8e82 (HEAD -> gicv2-apr-uaccess)
> Author: Christoffer Dall <cdall@xxxxxxxxxx>
> Date: Thu Aug 31 22:24:25 2017 +0200
>
> KVM: arm/arm64: Support uaccess of GICC_APRn
>
> When migrating guests around we need to know the active priorities to
> ensure functional virtual interrupt prioritization by the GIC.
>
> This commit clarifies the API and how active priorities of interrupts in
> different groups are represented, and implements the accessor functions
> for the uaccess register range.
>
> We live with a slight layering violation in accessing GICv3 data
> structures from vgic-mmio-v2.c, because anything else just adds too much
> complexity for us to deal with (it's not like there's a benefit
> elsewhere in the code of an intermediate representation as is the case
> with the VMCR). We accept this, because while doing v3 processing from
> a file named something-v2.c can look strange at first, this really is
> specific to dealing with the user space interface for something that
> looks like a GICv2.
>
> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx>
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b2f60ca..b3ce126 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -83,6 +83,11 @@ Groups:
>
> Bits for undefined preemption levels are RAZ/WI.
>
> + Note that this differs from a CPU's view of the APRs on hardware in which
> + a GIC without the security extensions expose group 0 and group 1 active
> + priorities in separate register groups, whereas we show a combined view
> + similar to GICv2's GICH_APR.
> +
> For historical reasons and to provide ABI compatibility with userspace we
> export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
> field in the lower 5 bits of a word, meaning that userspace must always
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 37522e6..b3d4a10 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -303,6 +303,51 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
> vgic_set_vmcr(vcpu, &vmcr);
> }
>
> +static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + int n; /* which APRn is this */
> +
> + n = (addr >> 2) & 0x3;
> +
> + if (kvm_vgic_global_state.type == VGIC_V2) {
> + /* GICv2 hardware systems support max. 32 groups */
> + if (n != 0)
> + return 0;
> + return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
> + } else {
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (n > vgic_v3_max_apr_idx(vcpu))
> + return 0;
> + /* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> + return vgicv3->vgic_ap1r[n];
> + }
> +}
> +
> +static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + int n; /* which APRn is this */
> +
> + n = (addr >> 2) & 0x3;
> +
> + if (kvm_vgic_global_state.type == VGIC_V2) {
> + /* GICv2 hardware systems support max. 32 groups */
> + if (n != 0)
> + return;
> + vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
> + } else {
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (n > vgic_v3_max_apr_idx(vcpu))
> + return;
> + /* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> + vgicv3->vgic_ap1r[n] = val;
> + }
> +}
> +
> static const struct vgic_register_region vgic_v2_dist_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
> vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> @@ -364,7 +409,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
> vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
> + vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
> vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>
For the record, I find this last solution quite elegant. Yes, the slight
violation of the v2/v3 separation is a bit annoying, but I can
understand this code in about 30 seconds as opposed to 30 minutes for
the original series.
So for these two patches:
Reviewed-by: Marc Zyngier <marc.zyngier@xxxxxxx>
Wanghaibin: Could you please give it a go and let us know if this solves
the issue for your particular test case?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
