Re: [Qemu-arm] [PATCH v3 16/20] intc/arm_gic: Implement gic_update_virt() function

[Luc Michel]

On 07/12/2018 03:56 PM, Peter Maydell wrote:
> On 29 June 2018 at 14:29, Luc Michel <address@hidden> wrote:
>> Add the gic_update_virt() function to update the vCPU interface states
>> and raise vIRQ and vFIQ as needed. This commit renames gic_update() to
>> gic_update_internal() and generalizes it to handle both cases, with a
>> `virt' parameter to track whether we are updating the CPU or vCPU
>> interfaces.
>>
>> The main difference between CPU and vCPU is the way we select the best
>> IRQ. This part has been split into the gic_get_best_(v)irq functions.
>> For the virt case, the LRs are iterated to find the best candidate.
>>
>> Signed-off-by: Luc Michel <address@hidden>
>> ---
>>  hw/intc/arm_gic.c | 170 +++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 130 insertions(+), 40 deletions(-)
> 
> 
>> +
>> +/* Return true if IRQ signaling is enabled:
>> + *   - !virt -> from the distributor to the CPU interfaces,
>> + *              for the given group mask,
>> + *   -  virt -> from the given virtual interface to the CPU virtual 
>> interface.
>> + */
>> +static inline bool gic_irq_signaling_enabled(GICState *s, int cpu, bool 
>> virt,
>> +                                    int group_mask)
>> +{
>> +    return (virt && (s->h_hcr[cpu] & R_GICH_HCR_EN_MASK))
>> +        || (!virt && (s->ctlr & group_mask));
>> +}
> 
> For a real CPU interface we test the GICC_CTLR EnableGrp0/1 bits here.
> For a virt CPU interface shouldn't we test the GICV_CTLR bits ?
This test is still done in gic_update_internal() (we test
s->cpu_ctlr[cpu_iface], with cpu_iface being the index of a vcpu when
virt is true). I can move it in this function if you think it's clearer?

> 
> (This doesn't actually cause any wrong behaviour because this check
> is just an efficiency check: "if interrupts are entirely disabled
> then we don't need to do the expensive look-at-all-irqs checks".)
> 
> 
> Otherwise looks OK.
> 
> thanks
> -- PMM
>

signature.asc
Description: OpenPGP digital signature