Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[Christoffer Dall <cdall@linaro.org>]

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@linaro.org>
> > > 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@linaro.org>
> 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@linaro.org>
> 
> 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@linaro.org>
> 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@linaro.org>
> 
> 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