Re: [Xen-devel] [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier <marc.zyngier@xxxxxxx>]

On 27/03/18 12:27, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 04:55 PM, Marc Zyngier wrote:
>> On 27/03/18 12:15, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 04:41 PM, Marc Zyngier wrote:
>>>> On 27/03/18 12:07, Manish Jaggi wrote:
>>>>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>>>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>>>>>> This patch is ported to xen from linux commit
>>>>>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
>>>>>>>>>>> ---
>>>>>>>>>>>       xen/arch/arm/arm64/vgic-v3-sr.c     | 70 
>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>>>>>       xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>>>>>       3 files changed, 77 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c 
>>>>>>>>>>> b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>>>>>        */
>>>>>>>>>>>       
>>>>>>>>>>>       #include <asm/current.h>
>>>>>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>>>>>       #include <asm/regs.h>
>>>>>>>>>>>       #include <asm/system.h>
>>>>>>>>>>>       #include <asm/traps.h>
>>>>>>>>>>>       
>>>>>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>>>>>> +
>>>>>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>>>>>> +{
>>>>>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>>>>>> +{
>>>>>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t 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 vgic_v3_read_bpr1(struct cpu_user_regs *regs, int 
>>>>>>>>>>> regidx)
>>>>>>>>>>> +{
>>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>>> +
>>>>>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int 
>>>>>>>>>>> regidx)
>>>>>>>>>>> +{
>>>>>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_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;
>>>>>>>>>>> +
>>>>>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const 
>>>>>>>>>>> union hsr hsr)
>>>>>>>>>>> +{
>>>>>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>>> +    else
>>>>>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>>       /*
>>>>>>>>>>>        * returns true if the register is emulated.
>>>>>>>>>>>        */
>>>>>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct 
>>>>>>>>>>> cpu_user_regs *regs)
>>>>>>>>>>>       
>>>>>>>>>>>           switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>>>>>           {
>>>>>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>>>>>> +         break;
>>>>>>>>>> What is the rational for indirecting through a function and moving 
>>>>>>>>>> the
>>>>>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>>>>>> change much, but since this is a port of existing code, it will make
>>>>>>>>>> more complex the port of potential fixes.
>>>>>>>>> I used xen template of handling sysreg traps
>>>>>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>>>>>> a handle_XXX function is used throughout...
>>>>>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>>>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>>>>>> decide.
>>>>>>>>
>>>>>>>> More importantly, my other question still stand: most trap functions do
>>>>>>>> require VMCR as an input. Why moving it to the leaf functions?
>>>>>>> Same reason, I was keeping the interface same of all handle_XXX 
>>>>>>> functions
>>>>>>> handle_XXX(regs, hsr, ...)
>>>>>>>
>>>>>>> Do you want me to change both to match with your patch or it is ok?
>>>>>> My preference would be to keep the code as initially written, as you're
>>>>>> pointlessly changing the flow. Again, that's for the maintainers to
>>>>>> comment, but you should at the very least indicate that change in the
>>>>>> commit log.
>>>>> I did in the cover letter
>>>> which, crucially, doesn't end-up in the commit. Oh well.
>>> I am working on to address the two points, will send v3 later today.
>>> - will remove emulate functions
>>> - and pass vmcr as a param.
>>>
>>> Will it be possible to review the remaining part of the code, so that I
>>> can address
>>> other comments in v3 as well.
>> I suggest you wait until some other folks have a chance to properly
>> review the series. You only posted the stuff this morning, give them a
>> chance. A week between two versions is probably the right timing.
> Xen 4.11 window closes this week, so I have only few days.
> I am hoping to get your ack before that.
My Ack is not the maintainers' (which you'd need anyway), and your
series is not the only one I need to review. I don't plan on having
another look at it this week anyway.

As for the Xen deadline, I'm sure there will be another one.

        M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel