Hi Wanghaibin,
On Fri, Sep 01, 2017 at 11:44:38AM +0200, 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,
>
>
I'd appreciate a tested-by on these patches with your configuration.
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
