Message ID | CAEpva6rMZu31dHDV1kN_XBjC4atJcDK-KxxL35A6XLTwcReySg@mail.gmail.com |
---|---|
State | New |
Headers | show |
On 7 December 2012 08:07, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote: > Sorry, it seems that I did not understand the flow in the function > "hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in > my previous patch. Please do not apply the previous patch, and apply > this one instead if you consider that it is correct. > > target-arm: fix bug in irq value on arm_gic.c > > Fix a small bug that was using an incorrect IRQ > value in the function gic_dist_writeb. > > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com> Thanks -- nice catch! The code change in this patch is correct, but there are a minor couple of formatting issues with it. If you can fix those and resend I can apply it to arm-devs.next. Firstly, you should send patches as emails which only include the commit message and patch (this one has a preamble 'Sorry...' and a quoted copy of your previous email); otherwise when they are applied this preamble ends up in the git commit log. Secondly, it would be good to be more specific about the problem being fixed (something like "Fix a bug where interrupts not set pending on the correct target CPUs when they are triggered by writes to the Interrupt Set Enable or Set Pending registers"). (git format-patch / git send-email send mail in the right format.) > --- > hw/arm_gic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/arm_gic.c b/hw/arm_gic.c > index f9e423f..64d4e23 100644 > --- a/hw/arm_gic.c > +++ b/hw/arm_gic.c > @@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, > value = 0xff; > for (i = 0; i < 8; i++) { > if (value & (1 << i)) { > - int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq); > + int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : > GIC_TARGET(irq + i); This line is too long. If you run scripts/checkpatch.pl on your patch you'll see that it complains about this. (Also, since your mailer is wrapping long lines, the result is a patch that doesn't apply cleanly or display properly in patchwork: http://patchwork.ozlabs.org/patch/204420/ ) I'd suggest a line break after the '='. > int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK; > > if (!GIC_TEST_ENABLED(irq + i, cm)) { > @@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, > > for (i = 0; i < 8; i++) { > if (value & (1 << i)) { > - GIC_SET_PENDING(irq + i, GIC_TARGET(irq)); > + GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i)); > } > } > } else if (offset < 0x300) { > -- If you don't have time to make these fixes that's OK -- I can fix the patch up in my tree. But they're good practice if you want to send more patches to QEMU in future ;-) -- PMM
Peter, Thanks for your kind explanation. I will fix the patch and send it again next Monday (hopefully I won't make any mistake this time). And yes, I would like to collaborate with more patches in the future so it might be good practice. In particular, I'm interested in helping to make current TrustZone support by Johannes Winter (https://github.com/jowinter/qemu-trustzone) mainstream. I can contribute by testing it or adding features that are not yet implemented. Best regards Daniel Sangorrin On Fri, Dec 7, 2012 at 9:16 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 December 2012 08:07, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote: >> Sorry, it seems that I did not understand the flow in the function >> "hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in >> my previous patch. Please do not apply the previous patch, and apply >> this one instead if you consider that it is correct. >> >> target-arm: fix bug in irq value on arm_gic.c >> >> Fix a small bug that was using an incorrect IRQ >> value in the function gic_dist_writeb. >> >> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com> > > Thanks -- nice catch! The code change in this patch is correct, > but there are a minor couple of formatting issues with it. > If you can fix those and resend I can apply it to arm-devs.next. > > Firstly, you should send patches as emails which only include > the commit message and patch (this one has a preamble 'Sorry...' > and a quoted copy of your previous email); otherwise when they > are applied this preamble ends up in the git commit log. > Secondly, it would be good to be more specific about the > problem being fixed (something like "Fix a bug where interrupts > not set pending on the correct target CPUs when they are > triggered by writes to the Interrupt Set Enable or Set Pending > registers"). > > (git format-patch / git send-email send mail in the right > format.) > >> --- >> hw/arm_gic.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/arm_gic.c b/hw/arm_gic.c >> index f9e423f..64d4e23 100644 >> --- a/hw/arm_gic.c >> +++ b/hw/arm_gic.c >> @@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, >> value = 0xff; >> for (i = 0; i < 8; i++) { >> if (value & (1 << i)) { >> - int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq); >> + int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : >> GIC_TARGET(irq + i); > > This line is too long. If you run scripts/checkpatch.pl on your patch > you'll see that it complains about this. (Also, since your mailer > is wrapping long lines, the result is a patch that doesn't apply > cleanly or display properly in patchwork: > http://patchwork.ozlabs.org/patch/204420/ > ) > > I'd suggest a line break after the '='. > >> int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK; >> >> if (!GIC_TEST_ENABLED(irq + i, cm)) { >> @@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, >> >> for (i = 0; i < 8; i++) { >> if (value & (1 << i)) { >> - GIC_SET_PENDING(irq + i, GIC_TARGET(irq)); >> + GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i)); >> } >> } >> } else if (offset < 0x300) { >> -- > > If you don't have time to make these fixes that's OK -- I can > fix the patch up in my tree. But they're good practice if you > want to send more patches to QEMU in future ;-) > > -- PMM
On 7 December 2012 14:31, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote: > Thanks for your kind explanation. I will fix the patch and send it > again next Monday (hopefully I won't make any mistake this time). And > yes, I would like to collaborate with more patches in the future so it > might be good practice. OK, cool. > In particular, I'm interested in helping to make current TrustZone > support by Johannes Winter > (https://github.com/jowinter/qemu-trustzone) mainstream. I can > contribute by testing it or adding features that are not yet > implemented. I'd certainly be interested in seeing proper TrustZone emulation support upstream if you and Johannes want to get it into shape to submit, yes. -- PMM
On 07.12.2012 15:38, Peter Maydell wrote: [...] > On 7 December 2012 14:31, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote: > [...] >> In particular, I'm interested in helping to make current TrustZone >> support by Johannes Winter >> (https://github.com/jowinter/qemu-trustzone) mainstream. I can >> contribute by testing it or adding features that are not yet >> implemented. > > I'd certainly be interested in seeing proper TrustZone emulation > support upstream if you and Johannes want to get it into shape > to submit, yes. > > -- PMM [...] I am definitely interested in bringing the TrustZone patches from my github fork of QEMU into a shape, which allow integration into upstream. Any input, help and feedback on that is highly appreciated. It would be great to have an outline on how this integration process could proceed. From my perspective it seems to be most appropriate to first focus on the patches which are immediately related to secure/normal world register banking and to the ARM-specific parts of QEMU's binary code translator. Kind regards, Johannes Winter
On Sat, Dec 8, 2012 at 12:58 AM, Johannes Winter <johannes.winter@iaik.tugraz.at> wrote: > On 07.12.2012 15:38, Peter Maydell wrote: > [...] > >> On 7 December 2012 14:31, Daniel Sangorrin <daniel.sangorrin@gmail.com> >> wrote: > >> [...] > >>> In particular, I'm interested in helping to make current TrustZone >>> support by Johannes Winter >>> (https://github.com/jowinter/qemu-trustzone) mainstream. I can >>> contribute by testing it or adding features that are not yet >>> implemented. >> >> >> I'd certainly be interested in seeing proper TrustZone emulation >> support upstream if you and Johannes want to get it into shape >> to submit, yes. >> >> -- PMM > > [...] > > I am definitely interested in bringing the TrustZone patches from my github > fork of QEMU into a shape, which allow integration into upstream. Any input, > help and feedback on that is highly appreciated. > > It would be great to have an outline on how this integration process could > proceed. From my perspective it seems to be most appropriate to first focus > on the patches which are immediately related to secure/normal world register > banking and to the ARM-specific parts of QEMU's binary code translator. > > Kind regards, > Johannes Winter Sorry for being silent during the last days. I was preparing the porting of our open-source Trustzone monitor (SafeG) to QEMU. The good news is that it is working and it can be downloaded here (it runs an RTOS called FMP on the trusted world and Linux on the non-trusted world). Note: I also have a smaller bare-metal test, which can be useful for small tests. http://www.toppers.jp/download.cgi/safeg-0.4-qemu-14dec2012.tar.gz I agree with you Johannes. Let's start simple. I will try to find time this month to do something about it. Best regards, Daniel
diff --git a/hw/arm_gic.c b/hw/arm_gic.c index f9e423f..64d4e23 100644 --- a/hw/arm_gic.c +++ b/hw/arm_gic.c @@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, value = 0xff; for (i = 0; i < 8; i++) { if (value & (1 << i)) { - int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq); + int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq + i); int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
Sorry, it seems that I did not understand the flow in the function "hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in my previous patch. Please do not apply the previous patch, and apply this one instead if you consider that it is correct. target-arm: fix bug in irq value on arm_gic.c Fix a small bug that was using an incorrect IRQ value in the function gic_dist_writeb. Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com> --- hw/arm_gic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) if (!GIC_TEST_ENABLED(irq + i, cm)) { @@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, for (i = 0; i < 8; i++) { if (value & (1 << i)) { - GIC_SET_PENDING(irq + i, GIC_TARGET(irq)); + GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i)); } } } else if (offset < 0x300) {