Re: [Qemu-ppc] [PATCH] openpic: Initialize destmask at reset

[Alexander Graf]

On 21.05.14 06:44, Paul Janzen wrote:
> The openpic emulation code maintains an allowable-CPU's bitmap
> ("destmask") for each IRQ which is calculated from the IDR register
> value whenever the guest OS writes to it.  However, if the guest OS
> relies on the system to set the IDR register to a default value at
> reset, and does not write IDR, then destmask does not get updated, and
> interrupts do not get propagated to the guest.  Initialize destmask from
> the provided idr_reset value in openpic_reset().
>
> Signed-off-by: Paul Janzen <address@hidden>

Welcome to the wonderful world of upstream qemu development :).

> ---
>   hw/intc/openpic.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index be76fbd..53aa006 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -551,6 +551,7 @@ static void openpic_reset(DeviceState *d)
>       for (i = 0; i < opp->max_irq; i++) {
>           opp->src[i].ivpr = opp->ivpr_reset;
>           opp->src[i].idr  = opp->idr_reset;
> +        opp->src[i].destmask = opp->idr_reset & ((1UL << opp->nb_cpus) - 1);

If I read the code correctly it should be safe to assume that no 
interrupt is set as critical on reset, so this calculation is safe. 
Could you please reassure me that you thought of this case and think the 
same? :)

Please replace the 1UL with a normal 1. The UL bit is legacy from when 
we used to have special bitmap code in openpic and it always makes me 
wary to see host long length sneak into emulation code - it's usually a 
recipe for breakage on 32bit hosts :).

Or maybe it's safer overall to just call write_IRQreg_idr() instead of 
setting idr directly? That would update destmask along the way as well 
and we would catch all subtle corner cases.

Do you have a simple test case for this patch? We seem to have the same 
bug in the in-kernel KVM MPIC emulation code and I'd like to have it 
fixed there as well, but I don't really like to do that change blindly.

Also, please CC address@hidden on version 2 of this patch. I can't 
apply patches that have only been posted to address@hidden, as they 
have not gone through public qemu review.


Thanks a lot for this patch and welcome again :)

Alex