On 6 June 2017 at 16:46, Christoffer Dall <cdall@xxxxxxxxxx> wrote:
> On Tue, Jun 06, 2017 at 04:15:05PM +0100, Marc Zyngier wrote:
>> >> +static void __hyp_text __vgic_v3_write_bpr0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>> >> +{
>> >> + u64 val = vcpu_get_reg(vcpu, rt);
>> >> + u8 bpr_min = 7 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
>> >> +
>> >> + /* Enforce BPR limiting */
>> >> + if (val < bpr_min)
>> >> + val = bpr_min;
>> >> +
>> >> + val <<= ICH_VMCR_BPR0_SHIFT;
>> >> + val &= ICH_VMCR_BPR0_MASK;
>> >> + vmcr &= ~ICH_VMCR_BPR0_MASK;
>> >> + vmcr |= val;
>> >> +
>> >> + if (vmcr & ICH_VMCR_CBPR_MASK) {
>> >> + val = __vgic_v3_get_bpr1(vmcr);
>> >> + val <<= ICH_VMCR_BPR1_SHIFT;
>> >> + val &= ICH_VMCR_BPR1_MASK;
>> >> + vmcr &= ~ICH_VMCR_BPR1_MASK;
>> >> + vmcr |= val;
>> >> + }
>> >
>> > I don't understand why this block is needed?
>>
>> If you have CBPR already set, and then update BPR0, you need to make
>> sure that BPR1 gets updated as well. You could hope that the HW would do
>> it for you, but since we're erratum workaround land...
>>
>
> I just didn't read the spec that way, I gathered that the hardware would
> maintain read-as-written for for bpr1 but use bpr0 to set the binary
> point when cbpr is set, and just ignore writes to bpr1 for as long as
> cbpr is set.
This depends whether you're talking about the ICC/ICV BPR0/BPR1 registers,
or the fields in the ICH_VMCR*. For the former, if CBPR is set then
BPR1 reads and writes affect BPR0. For the latter, the two fields
are both independent and read-as-written regardless of the value
of CBPR, it's just that the value in the BPR1 field has no effect.
(The reason for this is for VM state migration: if a guest does:
- write X to BPR0
- write Y to BPR1
- set CBPR
- write Z to BPR0
- unset CBPR
- read BPR1
it should get back Y still; so EL2 needs to have a way to see
both the underlying BPR0 and BPR1 values even if CPBR is in effect.)
So the code above is not correct, I think, because it's making
writes to BPR0 affect the BPR1 value. Instead what should happen
is that when we emulate reads and writes to ICC_BPR1 we should pay
attention to CBPR and work with either the VMCR BPR0 or BPR1 field
appropriately.
thanks
-- PMM
