diff mbox

[v2,05/25] KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Message ID 20170601102117.17750-6-marc.zyngier@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc Zyngier June 1, 2017, 10:20 a.m. UTC 
Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
register, which is located in the ICH_VMCR_EL2.BPR1 field.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/hyp/vgic-v3-sr.c | 51 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

Comments

Christoffer Dall June 4, 2017, 8:25 p.m. UTC | #1 
On Thu, Jun 01, 2017 at 11:20:57AM +0100, Marc Zyngier wrote:
> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
> register, which is located in the ICH_VMCR_EL2.BPR1 field.
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/hyp/vgic-v3-sr.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index 943bf11252d9..6254eaf72a77 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -375,6 +375,51 @@ void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
>  
>  #ifdef CONFIG_ARM64
>  
> +static unsigned int __hyp_text __vgic_v3_get_bpr0(u32 vmcr)
> +{
> +	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
> +}
> +
> +static unsigned int __hyp_text __vgic_v3_get_bpr1(u32 vmcr)
> +{
> +	unsigned int bpr;
> +
> +	if (vmcr & ICH_VMCR_CBPR_MASK) {
> +		bpr = __vgic_v3_get_bpr0(vmcr);
> +		if (bpr < 7)
> +			bpr++;
> +	} else {
> +		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
> +	}
> +
> +	return bpr;
> +}
> +
> +static void __hyp_text __vgic_v3_read_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> +{
> +	vcpu_set_reg(vcpu, rt, __vgic_v3_get_bpr1(vmcr));
> +}
> +
> +static void __hyp_text __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> +{
> +	u64 val = vcpu_get_reg(vcpu, rt);
> +	u8 bpr_min = 8 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));

I can't seem to find where this behavior is documented, is it that 8 is
the theoretical max, and it's the upper preemption levels that apply, so
it must be 8 - number supported?

> +
> +	if (vmcr & ICH_VMCR_CBPR_MASK)
> +		return;
> +
> +	/* Enforce BPR limiting */
> +	if (val < bpr_min)
> +		val = bpr_min;

Are we not implying that the reset value here also means bpr_min?  I
also can't seem to find this behavior in the spec and it appears we rely
on the underlying hardware to set a reset value (by setting vmcr=0 on
first run).  Could this result in a guest observing one reset value,
writing 0 to this register, and observing another one?

> +
> +	val <<= ICH_VMCR_BPR1_SHIFT;
> +	val &= ICH_VMCR_BPR1_MASK;
> +	vmcr &= ~ICH_VMCR_BPR1_MASK;
> +	vmcr |= val;
> +
> +	__vgic_v3_write_vmcr(vmcr);
> +}
> +
>  int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
>  {
>  	int rt;
> @@ -397,6 +442,12 @@ int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
>  	is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ;
>  
>  	switch (sysreg) {
> +	case SYS_ICC_BPR1_EL1:
> +		if (is_read)
> +			fn = __vgic_v3_read_bpr1;
> +		else
> +			fn = __vgic_v3_write_bpr1;
> +		break;

Did you consider a lookup table with the sysreg encoding, the read
function, and the right function as each entry?  It may compress the
code a bit, but I'm not sure if it's nicer or worth it.

>  	default:
>  		return 0;
>  	}
> -- 
> 2.11.0
> 

Thanks,
-Christoffer
Marc Zyngier June 5, 2017, 9:58 a.m. UTC | #2 
On 04/06/17 21:25, Christoffer Dall wrote:
> On Thu, Jun 01, 2017 at 11:20:57AM +0100, Marc Zyngier wrote:
>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/hyp/vgic-v3-sr.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> index 943bf11252d9..6254eaf72a77 100644
>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> @@ -375,6 +375,51 @@ void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
>>  
>>  #ifdef CONFIG_ARM64
>>  
>> +static unsigned int __hyp_text __vgic_v3_get_bpr0(u32 vmcr)
>> +{
>> +	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>> +}
>> +
>> +static unsigned int __hyp_text __vgic_v3_get_bpr1(u32 vmcr)
>> +{
>> +	unsigned int bpr;
>> +
>> +	if (vmcr & ICH_VMCR_CBPR_MASK) {
>> +		bpr = __vgic_v3_get_bpr0(vmcr);
>> +		if (bpr < 7)
>> +			bpr++;
>> +	} else {
>> +		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>> +	}
>> +
>> +	return bpr;
>> +}
>> +
>> +static void __hyp_text __vgic_v3_read_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>> +{
>> +	vcpu_set_reg(vcpu, rt, __vgic_v3_get_bpr1(vmcr));
>> +}
>> +
>> +static void __hyp_text __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>> +{
>> +	u64 val = vcpu_get_reg(vcpu, rt);
>> +	u8 bpr_min = 8 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
> 
> I can't seem to find where this behavior is documented, is it that 8 is
> the theoretical max, and it's the upper preemption levels that apply, so
> it must be 8 - number supported?

I took inspiration from the VPriorityGroup() helper in the GICv3
pseudocode. You can also deduct this from the table described in the
ICC_BPR0_EL1 documentation, though that's admittedly not very clear.

>> +
>> +	if (vmcr & ICH_VMCR_CBPR_MASK)
>> +		return;
>> +
>> +	/* Enforce BPR limiting */
>> +	if (val < bpr_min)
>> +		val = bpr_min;
> 
> Are we not implying that the reset value here also means bpr_min?  I
> also can't seem to find this behavior in the spec and it appears we rely
> on the underlying hardware to set a reset value (by setting vmcr=0 on
> first run).  Could this result in a guest observing one reset value,
> writing 0 to this register, and observing another one?

From the ICC_BPR0_EL1 documentation:

"An attempt to program the binary point field to a value less than the
minimum value sets the field to the minimum value."

That's the only way you can find out about the minimum value (and the
kernel also uses this functionality to be in a known state, even if it
doesn't use preemption yet).

> 
>> +
>> +	val <<= ICH_VMCR_BPR1_SHIFT;
>> +	val &= ICH_VMCR_BPR1_MASK;
>> +	vmcr &= ~ICH_VMCR_BPR1_MASK;
>> +	vmcr |= val;
>> +
>> +	__vgic_v3_write_vmcr(vmcr);
>> +}
>> +
>>  int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
>>  {
>>  	int rt;
>> @@ -397,6 +442,12 @@ int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
>>  	is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ;
>>  
>>  	switch (sysreg) {
>> +	case SYS_ICC_BPR1_EL1:
>> +		if (is_read)
>> +			fn = __vgic_v3_read_bpr1;
>> +		else
>> +			fn = __vgic_v3_write_bpr1;
>> +		break;
> 
> Did you consider a lookup table with the sysreg encoding, the read
> function, and the right function as each entry?  It may compress the
> code a bit, but I'm not sure if it's nicer or worth it.

I did think of it, but I wasn't sure how much nicer that would be. Happy
to try it and see if that's an improvement though.

Thanks,

	M.
Christoffer Dall June 5, 2017, 10:16 a.m. UTC | #3 
On Mon, Jun 05, 2017 at 10:58:57AM +0100, Marc Zyngier wrote:
> On 04/06/17 21:25, Christoffer Dall wrote:
> > On Thu, Jun 01, 2017 at 11:20:57AM +0100, Marc Zyngier wrote:
> >> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
> >> register, which is located in the ICH_VMCR_EL2.BPR1 field.
> >>
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  virt/kvm/arm/hyp/vgic-v3-sr.c | 51 +++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> >> index 943bf11252d9..6254eaf72a77 100644
> >> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> >> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> >> @@ -375,6 +375,51 @@ void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
> >>  
> >>  #ifdef CONFIG_ARM64
> >>  
> >> +static unsigned int __hyp_text __vgic_v3_get_bpr0(u32 vmcr)
> >> +{
> >> +	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
> >> +}
> >> +
> >> +static unsigned int __hyp_text __vgic_v3_get_bpr1(u32 vmcr)
> >> +{
> >> +	unsigned int bpr;
> >> +
> >> +	if (vmcr & ICH_VMCR_CBPR_MASK) {
> >> +		bpr = __vgic_v3_get_bpr0(vmcr);
> >> +		if (bpr < 7)
> >> +			bpr++;
> >> +	} else {
> >> +		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
> >> +	}
> >> +
> >> +	return bpr;
> >> +}
> >> +
> >> +static void __hyp_text __vgic_v3_read_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> >> +{
> >> +	vcpu_set_reg(vcpu, rt, __vgic_v3_get_bpr1(vmcr));
> >> +}
> >> +
> >> +static void __hyp_text __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> >> +{
> >> +	u64 val = vcpu_get_reg(vcpu, rt);
> >> +	u8 bpr_min = 8 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
> > 
> > I can't seem to find where this behavior is documented, is it that 8 is
> > the theoretical max, and it's the upper preemption levels that apply, so
> > it must be 8 - number supported?
> 
> I took inspiration from the VPriorityGroup() helper in the GICv3
> pseudocode. You can also deduct this from the table described in the
> ICC_BPR0_EL1 documentation, though that's admittedly not very clear.
> 
> >> +
> >> +	if (vmcr & ICH_VMCR_CBPR_MASK)
> >> +		return;
> >> +
> >> +	/* Enforce BPR limiting */
> >> +	if (val < bpr_min)
> >> +		val = bpr_min;
> > 
> > Are we not implying that the reset value here also means bpr_min?  I
> > also can't seem to find this behavior in the spec and it appears we rely
> > on the underlying hardware to set a reset value (by setting vmcr=0 on
> > first run).  Could this result in a guest observing one reset value,
> > writing 0 to this register, and observing another one?
> 
> From the ICC_BPR0_EL1 documentation:
> 
> "An attempt to program the binary point field to a value less than the
> minimum value sets the field to the minimum value."
> 
> That's the only way you can find out about the minimum value (and the
> kernel also uses this functionality to be in a known state, even if it
> doesn't use preemption yet).
> 

Hmm, the ICV_BPR0_EL1 says:
  "An attempt to program the binary point field to a value less than the
  minimum value sets the field to the minimum value. On a reset, the
  binary point field is set to the minimum supported value."

But then the ICV_BPR1_EL1 says:
  "An attempt to program the binary point field to a value less than the
  reset value sets the field to the reset value."

  and

  "Writing 0 to this field will set this field to its reset value, which
  is IMPLEMENTATION DEFINED and non-zero."

So it seems like the spec is making some distinction between minimum or
reset value and between the two registers, but I'm not sure if the spec
is just having its fun with us, or if there's something important to be
aware of here.


> > 
> >> +
> >> +	val <<= ICH_VMCR_BPR1_SHIFT;
> >> +	val &= ICH_VMCR_BPR1_MASK;
> >> +	vmcr &= ~ICH_VMCR_BPR1_MASK;
> >> +	vmcr |= val;
> >> +
> >> +	__vgic_v3_write_vmcr(vmcr);
> >> +}
> >> +
> >>  int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
> >>  {
> >>  	int rt;
> >> @@ -397,6 +442,12 @@ int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
> >>  	is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ;
> >>  
> >>  	switch (sysreg) {
> >> +	case SYS_ICC_BPR1_EL1:
> >> +		if (is_read)
> >> +			fn = __vgic_v3_read_bpr1;
> >> +		else
> >> +			fn = __vgic_v3_write_bpr1;
> >> +		break;
> > 
> > Did you consider a lookup table with the sysreg encoding, the read
> > function, and the right function as each entry?  It may compress the
> > code a bit, but I'm not sure if it's nicer or worth it.
> 
> I did think of it, but I wasn't sure how much nicer that would be. Happy
> to try it and see if that's an improvement though.
> 

Meh, probably not worth it.

Thanks,
-Christoffer
Peter Maydell June 5, 2017, 10:27 a.m. UTC | #4 
On 5 June 2017 at 11:16, Christoffer Dall <cdall@linaro.org> wrote:
> On Mon, Jun 05, 2017 at 10:58:57AM +0100, Marc Zyngier wrote:
>> From the ICC_BPR0_EL1 documentation:
>>
>> "An attempt to program the binary point field to a value less than the
>> minimum value sets the field to the minimum value."
>>
>> That's the only way you can find out about the minimum value (and the
>> kernel also uses this functionality to be in a known state, even if it
>> doesn't use preemption yet).
>>
>
> Hmm, the ICV_BPR0_EL1 says:
>   "An attempt to program the binary point field to a value less than the
>   minimum value sets the field to the minimum value. On a reset, the
>   binary point field is set to the minimum supported value."
>
> But then the ICV_BPR1_EL1 says:
>   "An attempt to program the binary point field to a value less than the
>   reset value sets the field to the reset value."
>
>   and
>
>   "Writing 0 to this field will set this field to its reset value, which
>   is IMPLEMENTATION DEFINED and non-zero."

It also later says that the ICV_BPR1_EL1 IMPDEF reset value is always
the ICV_BPR0_EL1 reset value plus one.

> So it seems like the spec is making some distinction between minimum or
> reset value and between the two registers, but I'm not sure if the spec
> is just having its fun with us, or if there's something important to be
> aware of here.

I think the plus-one is the only interesting difference and the
rest is just accidental difference of wording.

There is a difference between ICV_BPR* and ICC_BPR*, which is that
the latter reset to an UNKNOWN value, not necessarily the minimum.

thanks
-- PMM
Christoffer Dall June 6, 2017, 9:41 a.m. UTC | #5 
On Mon, Jun 05, 2017 at 10:58:57AM +0100, Marc Zyngier wrote:
> On 04/06/17 21:25, Christoffer Dall wrote:
> > On Thu, Jun 01, 2017 at 11:20:57AM +0100, Marc Zyngier wrote:
> >> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
> >> register, which is located in the ICH_VMCR_EL2.BPR1 field.
> >>
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  virt/kvm/arm/hyp/vgic-v3-sr.c | 51 +++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> >> index 943bf11252d9..6254eaf72a77 100644
> >> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> >> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> >> @@ -375,6 +375,51 @@ void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
> >>  
> >>  #ifdef CONFIG_ARM64
> >>  
> >> +static unsigned int __hyp_text __vgic_v3_get_bpr0(u32 vmcr)
> >> +{
> >> +	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
> >> +}
> >> +
> >> +static unsigned int __hyp_text __vgic_v3_get_bpr1(u32 vmcr)
> >> +{
> >> +	unsigned int bpr;
> >> +
> >> +	if (vmcr & ICH_VMCR_CBPR_MASK) {
> >> +		bpr = __vgic_v3_get_bpr0(vmcr);
> >> +		if (bpr < 7)
> >> +			bpr++;
> >> +	} else {
> >> +		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
> >> +	}
> >> +
> >> +	return bpr;
> >> +}
> >> +
> >> +static void __hyp_text __vgic_v3_read_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> >> +{
> >> +	vcpu_set_reg(vcpu, rt, __vgic_v3_get_bpr1(vmcr));
> >> +}
> >> +
> >> +static void __hyp_text __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> >> +{
> >> +	u64 val = vcpu_get_reg(vcpu, rt);
> >> +	u8 bpr_min = 8 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
> > 
> > I can't seem to find where this behavior is documented, is it that 8 is
> > the theoretical max, and it's the upper preemption levels that apply, so
> > it must be 8 - number supported?
> 
> I took inspiration from the VPriorityGroup() helper in the GICv3
> pseudocode. You can also deduct this from the table described in the
> ICC_BPR0_EL1 documentation, though that's admittedly not very clear.
> 

Ah, yes, now I understand this. Since you can at most support
ICH_VTR_EL2.PREbits than the minimal value must be one that doesn't
define more preemmption levels.  I don't know why this was so hard for
me to realize.

Thanks,
-Christoffer
diff mbox

Patch

diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index 943bf11252d9..6254eaf72a77 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -375,6 +375,51 @@  void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
 
 #ifdef CONFIG_ARM64
 
+static unsigned int __hyp_text __vgic_v3_get_bpr0(u32 vmcr)
+{
+	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int __hyp_text __vgic_v3_get_bpr1(u32 vmcr)
+{
+	unsigned int bpr;
+
+	if (vmcr & ICH_VMCR_CBPR_MASK) {
+		bpr = __vgic_v3_get_bpr0(vmcr);
+		if (bpr < 7)
+			bpr++;
+	} else {
+		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+	}
+
+	return bpr;
+}
+
+static void __hyp_text __vgic_v3_read_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
+{
+	vcpu_set_reg(vcpu, rt, __vgic_v3_get_bpr1(vmcr));
+}
+
+static void __hyp_text __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
+{
+	u64 val = vcpu_get_reg(vcpu, rt);
+	u8 bpr_min = 8 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
+
+	if (vmcr & ICH_VMCR_CBPR_MASK)
+		return;
+
+	/* Enforce BPR limiting */
+	if (val < bpr_min)
+		val = bpr_min;
+
+	val <<= ICH_VMCR_BPR1_SHIFT;
+	val &= ICH_VMCR_BPR1_MASK;
+	vmcr &= ~ICH_VMCR_BPR1_MASK;
+	vmcr |= val;
+
+	__vgic_v3_write_vmcr(vmcr);
+}
+
 int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
 {
 	int rt;
@@ -397,6 +442,12 @@  int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
 	is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ;
 
 	switch (sysreg) {
+	case SYS_ICC_BPR1_EL1:
+		if (is_read)
+			fn = __vgic_v3_read_bpr1;
+		else
+			fn = __vgic_v3_write_bpr1;
+		break;
 	default:
 		return 0;
 	}