Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On Thu, 28 Nov 2019, 18:09 Stefano Stabellini, wrote: > On Thu, 28 Nov 2019, Julien Grall wrote: > > > In both cases I see no reason to keep wrong code. > > > > > > Either the patch will let run Linux 5.4 fine - then the patch should > > > definitely be taken. > > That's up to Stefano and Peng to provide me information why this is fine. > > FAOD, the current justification provided is not acceptable for me. > > I disagree. This is a typo fix. The original design was never spec > compliant. You cannot expect the typo fix to explain why the original > behavior is tolerable. That is out of scope and should *not* be required > for this fix. May I remind you that as a maintainer, this is in my right to say no to a patch. > We cannot expect typo fixes to go and trigger vgic/gic reworks and deep > investigations. This is a wrong expectation now, and going forward. > That's the best way to turn Xen into a bunch of hacks. I pointed out several times a potential issue with this patch. I also spent some part of my week-end investigating it and provide some insight. Did you look at them? If you want this patch in, then please help explaining why 5.4 is going to run fine on Xen 4.13 rather than keeping arguing this is a typo fix. Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On Thu, 28 Nov 2019, Julien Grall wrote: > > In both cases I see no reason to keep wrong code. > > > > Either the patch will let run Linux 5.4 fine - then the patch should > > definitely be taken. > That's up to Stefano and Peng to provide me information why this is fine. > FAOD, the current justification provided is not acceptable for me. I disagree. This is a typo fix. The original design was never spec compliant. You cannot expect the typo fix to explain why the original behavior is tolerable. That is out of scope and should *not* be required for this fix. We cannot expect typo fixes to go and trigger vgic/gic reworks and deep investigations. This is a wrong expectation now, and going forward. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On 28/11/2019 09:00, Jürgen Groß wrote: On 28.11.19 09:48, Julien Grall wrote: Hi, On 28/11/2019 08:32, Jürgen Groß wrote: On 28.11.19 09:14, Julien Grall wrote: In short, I think the patch should go in now and there are no downsides to it. That's it, I rest my case. Julien, I hope you'll reconsider. I don't really see the point to try to allow Linux 5.4 booting on Xen 4.13 without knowing whether we are not going to uncovered more BUG around I*ACTIVER. Sorry, but this is a rather weird statement. IIUC you are saying that a typo blocking boot of Linux 5.4 should not be fixed as you are not sure there are no other bugs? The implementation of I*ACTIVER was incorrect but gone unnoticed because no-one used it until 5.4. It also happen that we didn't cover all the I*ACTIVER registers, so 5.4 crashes instead of using the wrong behavior. This patch is basically replacing a guest crash by a behavior we don't fully understand. If you really want this patch in Xen 4.13, then you should read my mail on the first version and trying to answer me why this 5.4 is going to be safe running on Xen 4.13. Or do you think that with the typo fixed and running Linux 5.4 guests will destabilize the host? It is not going to destabilize the hosts. But this is not going to make 5.4 running correctly as Xen guest. Have you verified it isn't running correctly or do you just think it could hit problems? I haven't tested myself, but any bug around vGIC is usually subtled. I wrote a long e-mail on v1 (see [1]) explaning what could happen. To summarize briefly, Linux is reading the I*ACTIVER registers to check whether an interrupt is active at the hardware level. For instance, this is used to ensure all active interrupts have been handled before continuing. By always returning 0, we tell Linux there are no interrupts. One of the risk is interrupts may be lost. But that's Linux behavior, I can't tell how this is going to be used by others OSes. In both cases I see no reason to keep wrong code. Either the patch will let run Linux 5.4 fine - then the patch should definitely be taken. That's up to Stefano and Peng to provide me information why this is fine. FAOD, the current justification provided is not acceptable for me. Or the patch will let Linux 5.4 boot further, but some problems will occur. Then it will be possible to analyze those problems and try to fix them, very possibly with the sane approach you are hoping for. Juergen [1] https://lore.kernel.org/xen-devel/7289f75f-1ab2-2d42-cd88-1be5306b3...@xen.org/ -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On 28.11.19 09:48, Julien Grall wrote: Hi, On 28/11/2019 08:32, Jürgen Groß wrote: On 28.11.19 09:14, Julien Grall wrote: In short, I think the patch should go in now and there are no downsides to it. That's it, I rest my case. Julien, I hope you'll reconsider. I don't really see the point to try to allow Linux 5.4 booting on Xen 4.13 without knowing whether we are not going to uncovered more BUG around I*ACTIVER. Sorry, but this is a rather weird statement. IIUC you are saying that a typo blocking boot of Linux 5.4 should not be fixed as you are not sure there are no other bugs? The implementation of I*ACTIVER was incorrect but gone unnoticed because no-one used it until 5.4. It also happen that we didn't cover all the I*ACTIVER registers, so 5.4 crashes instead of using the wrong behavior. This patch is basically replacing a guest crash by a behavior we don't fully understand. If you really want this patch in Xen 4.13, then you should read my mail on the first version and trying to answer me why this 5.4 is going to be safe running on Xen 4.13. Or do you think that with the typo fixed and running Linux 5.4 guests will destabilize the host? It is not going to destabilize the hosts. But this is not going to make 5.4 running correctly as Xen guest. Have you verified it isn't running correctly or do you just think it could hit problems? In both cases I see no reason to keep wrong code. Either the patch will let run Linux 5.4 fine - then the patch should definitely be taken. Or the patch will let Linux 5.4 boot further, but some problems will occur. Then it will be possible to analyze those problems and try to fix them, very possibly with the sane approach you are hoping for. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
Hi, On 28/11/2019 08:32, Jürgen Groß wrote: On 28.11.19 09:14, Julien Grall wrote: In short, I think the patch should go in now and there are no downsides to it. That's it, I rest my case. Julien, I hope you'll reconsider. I don't really see the point to try to allow Linux 5.4 booting on Xen 4.13 without knowing whether we are not going to uncovered more BUG around I*ACTIVER. Sorry, but this is a rather weird statement. IIUC you are saying that a typo blocking boot of Linux 5.4 should not be fixed as you are not sure there are no other bugs? The implementation of I*ACTIVER was incorrect but gone unnoticed because no-one used it until 5.4. It also happen that we didn't cover all the I*ACTIVER registers, so 5.4 crashes instead of using the wrong behavior. This patch is basically replacing a guest crash by a behavior we don't fully understand. If you really want this patch in Xen 4.13, then you should read my mail on the first version and trying to answer me why this 5.4 is going to be safe running on Xen 4.13. Or do you think that with the typo fixed and running Linux 5.4 guests will destabilize the host? It is not going to destabilize the hosts. But this is not going to make 5.4 running correctly as Xen guest. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
> Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER > range > > Hi Stefano, > > On 28/11/2019 01:12, Stefano Stabellini wrote: > > On Wed, 27 Nov 2019, Jürgen Groß wrote: > >> On 27.11.19 10:31, Peng Fan wrote: > >>>> Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix > >>>> GICD_ISACTIVER range > >>>> > >>>> On 27.11.19 01:01, Julien Grall wrote: > >>>>> Hi, > >>>>> > >>>>> On 26/11/2019 23:17, Stefano Stabellini wrote: > >>>>>> On Tue, 26 Nov 2019, Julien Grall wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On 26/11/2019 20:43, Stefano Stabellini wrote: > >>>>>>>> + Juergen > >>>>>>>> > >>>>>>>> I missed that you weren't in CC to the original patch, sorry. > >>>>>>>> I think this patch should go in, as otherwise Linux 5.4 could > >>>>>>>> run into problems. It is also a pretty straightforward 4 lines patch. > >>>>>>> > >>>>>>> 5.5 (or 5.6) is not going to run on Xen for other reasons (still > >>>>>>> in the vGIC)... So I would not view this as critical. > >>>>>> > >>>>>> 5.5 is not out yet, in fact, the dev window has just opened. > >>>>>> Isn't your statement a bit premature? > >>>>> > >>>>> The GICv4.1 work [1] is going to prevent Linux booting on all > >>>>> current versions of Xen. While I can't confirm this is going to be > >>>>> merged in 5.5, I can tell you this will break. > >>>>> > >>>>>> > >>>>>> In any case, even if potential future Linux releases could have > >>>>>> other additional issues, I don't think it should change our > >>>>>> current view on this specific issue which affects 5.4, just released. > >>>>> > >>>>> The patch is definitely not as straightforward as you may think. > >>>>> Please refer to the discussion we had on the first version. I > >>>>> voiced concern about this approach and gave point what could go > >>>>> wrong with > >>>> happen. > >>>>> > >>>>> This patch may be better than the current state (i.e crashing), > >>>>> but this wasn't tested enough to confirm this is the correct > >>>>> things to do and no other bug will appear (I don't believe reading > >>>>> I*ACTIVER was ever tested before). > >>>>> > >>>>> It is an annoying bug, but this is only affecting 5.4 which has > >>>>> just been released. It feels to me this is a fairly risky choice > >>>>> to merge it qutie late in the release without a good graps of the > >>>>> problem (see above). > >>>>> > >>>>> So I would definitly, prefer if this patch is getting through > >>>>> backport once we get more testing. > >>>>> > >>>>> We can still document the bug in the release note and point people > >>>>> to the patch. > >>>>> > >>>>> Anyway, this is Juergen choice here. But at least now he has the > >>>>> full picture... > >>>>> > >>>>> Cheers, > >>>>> > >>>>> [1] > >>>>> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. > >>>>> > >>>> > net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co > >>>> m%7Cdca > >>>>> > >>>> > dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 > >>>> 5%7C0%7 > >>>>> > >>>> > C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv > >>>> d73xb5 > >>>>> 2d6ERVQ%3Dreserved=0 > >>>>> > >>>> > >>>> Thanks, Julien, for sharing your opinion. > >>>> > >>>> With that statement I'd like to defer this patch to 4.14. > >>> > >>> But without this patch, 5.4 kernel will crash. So you prefer we > >>> develop the solution as Julien suggested for 4.13? > >> > >> I certainly won't take a patch for 4.13 when a maintainer of the > &g
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On 28.11.19 09:14, Julien Grall wrote: Hi Stefano, On 28/11/2019 01:12, Stefano Stabellini wrote: On Wed, 27 Nov 2019, Jürgen Groß wrote: On 27.11.19 10:31, Peng Fan wrote: Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range On 27.11.19 01:01, Julien Grall wrote: Hi, On 26/11/2019 23:17, Stefano Stabellini wrote: On Tue, 26 Nov 2019, Julien Grall wrote: Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? The GICv4.1 work [1] is going to prevent Linux booting on all current versions of Xen. While I can't confirm this is going to be merged in 5.5, I can tell you this will break. In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. The patch is definitely not as straightforward as you may think. Please refer to the discussion we had on the first version. I voiced concern about this approach and gave point what could go wrong with happen. This patch may be better than the current state (i.e crashing), but this wasn't tested enough to confirm this is the correct things to do and no other bug will appear (I don't believe reading I*ACTIVER was ever tested before). It is an annoying bug, but this is only affecting 5.4 which has just been released. It feels to me this is a fairly risky choice to merge it qutie late in the release without a good graps of the problem (see above). So I would definitly, prefer if this patch is getting through backport once we get more testing. We can still document the bug in the release note and point people to the patch. Anyway, this is Juergen choice here. But at least now he has the full picture... Cheers, [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co m%7Cdca dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 5%7C0%7 C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv d73xb5 2d6ERVQ%3Dreserved=0 Thanks, Julien, for sharing your opinion. With that statement I'd like to defer this patch to 4.14. But without this patch, 5.4 kernel will crash. So you prefer we develop the solution as Julien suggested for 4.13? I certainly won't take a patch for 4.13 when a maintainer of the related code has reservations against it. I think the best thing to do is to develop a proper patch the maintainers are happy with and don't try to force it into 4.13 now. Such a patch can still be backported to 4.13 later. I chatted with Juergen and he explained to me something I didn't know before. The release manager can only *block* a patch from being committed, he/she cannot actually decide if the patch should be committed or not for a given release. He/she cannot overrule a maintainer either. In this case, Juergen cannot make the decision on whether the patch should go in 4.13 or not. Although I couldn't reproduce the problem on Xilinx boards, I have to take the community angle on this, and I would like to make sure our releases work properly on any hardware, including NXP. Thus, I'll make the case one more time, hoping that Julien might change his mind :-) We had promise that patches to support NXP will be upstreamed, but this was never done. If you look at [1], there are a lot of patches on top of it. So I don't think NXP boot out-of-box and therefore I don't think we should make the decision based on this. We know that the bug fix won't introduce any regressions because, as Julien wrote, this code path was never used before. Also because of that, waiting for the backport and more OSSTest runs won't make much of a difference because OSSTest won't exercise this code path. Conversely, we don't know how many regression this is going to be introducing for Linux 5.4 because this was only reproduced once and we know the implementation is incorrect. But OSSTest is also testing different version of Linux and 5.4 should be tested soon (if not already), so we should also be able to exercise this code path. It is true that the original code handling GICD_ISACTIVER was never spec compliant, and it should be fixed properly. However, that is not what this patch addresses. That code, in addition from not being spec compliant by design, it also happens to have a typo. Fixing the typo at this stage of the release is appropriate at least to get a consistent behavior in the handling of GICD_ISACTIVER*, and also for Linux 5.4 as guest. Not to give
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
Hi Stefano, On 28/11/2019 01:12, Stefano Stabellini wrote: On Wed, 27 Nov 2019, Jürgen Groß wrote: On 27.11.19 10:31, Peng Fan wrote: Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range On 27.11.19 01:01, Julien Grall wrote: Hi, On 26/11/2019 23:17, Stefano Stabellini wrote: On Tue, 26 Nov 2019, Julien Grall wrote: Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? The GICv4.1 work [1] is going to prevent Linux booting on all current versions of Xen. While I can't confirm this is going to be merged in 5.5, I can tell you this will break. In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. The patch is definitely not as straightforward as you may think. Please refer to the discussion we had on the first version. I voiced concern about this approach and gave point what could go wrong with happen. This patch may be better than the current state (i.e crashing), but this wasn't tested enough to confirm this is the correct things to do and no other bug will appear (I don't believe reading I*ACTIVER was ever tested before). It is an annoying bug, but this is only affecting 5.4 which has just been released. It feels to me this is a fairly risky choice to merge it qutie late in the release without a good graps of the problem (see above). So I would definitly, prefer if this patch is getting through backport once we get more testing. We can still document the bug in the release note and point people to the patch. Anyway, this is Juergen choice here. But at least now he has the full picture... Cheers, [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co m%7Cdca dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 5%7C0%7 C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv d73xb5 2d6ERVQ%3Dreserved=0 Thanks, Julien, for sharing your opinion. With that statement I'd like to defer this patch to 4.14. But without this patch, 5.4 kernel will crash. So you prefer we develop the solution as Julien suggested for 4.13? I certainly won't take a patch for 4.13 when a maintainer of the related code has reservations against it. I think the best thing to do is to develop a proper patch the maintainers are happy with and don't try to force it into 4.13 now. Such a patch can still be backported to 4.13 later. I chatted with Juergen and he explained to me something I didn't know before. The release manager can only *block* a patch from being committed, he/she cannot actually decide if the patch should be committed or not for a given release. He/she cannot overrule a maintainer either. In this case, Juergen cannot make the decision on whether the patch should go in 4.13 or not. Although I couldn't reproduce the problem on Xilinx boards, I have to take the community angle on this, and I would like to make sure our releases work properly on any hardware, including NXP. Thus, I'll make the case one more time, hoping that Julien might change his mind :-) We had promise that patches to support NXP will be upstreamed, but this was never done. If you look at [1], there are a lot of patches on top of it. So I don't think NXP boot out-of-box and therefore I don't think we should make the decision based on this. We know that the bug fix won't introduce any regressions because, as Julien wrote, this code path was never used before. Also because of that, waiting for the backport and more OSSTest runs won't make much of a difference because OSSTest won't exercise this code path. Conversely, we don't know how many regression this is going to be introducing for Linux 5.4 because this was only reproduced once and we know the implementation is incorrect. But OSSTest is also testing different version of Linux and 5.4 should be tested soon (if not already), so we should also be able to exercise this code path. It is true that the original code handling GICD_ISACTIVER was never spec compliant, and it should be fixed properly. However, that is not what this patch addresses. That code, in addition from not being spec compliant by design, it also happens to have a typo. Fixing the typo at this stage of the release is appropriate at least to get a consistent behavior in the handling of GICD_ISACTIVER*, and also for Linux 5.4 as guest. Not to give a false impression to users, the warning ensures
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On Wed, 27 Nov 2019, Jürgen Groß wrote: > On 27.11.19 10:31, Peng Fan wrote: > > > Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER > > > range > > > > > > On 27.11.19 01:01, Julien Grall wrote: > > > > Hi, > > > > > > > > On 26/11/2019 23:17, Stefano Stabellini wrote: > > > > > On Tue, 26 Nov 2019, Julien Grall wrote: > > > > > > Hi, > > > > > > > > > > > > On 26/11/2019 20:43, Stefano Stabellini wrote: > > > > > > > + Juergen > > > > > > > > > > > > > > I missed that you weren't in CC to the original patch, sorry. > > > > > > > I think this patch should go in, as otherwise Linux 5.4 could run > > > > > > > into problems. It is also a pretty straightforward 4 lines patch. > > > > > > > > > > > > 5.5 (or 5.6) is not going to run on Xen for other reasons (still in > > > > > > the vGIC)... So I would not view this as critical. > > > > > > > > > > 5.5 is not out yet, in fact, the dev window has just opened. Isn't > > > > > your statement a bit premature? > > > > > > > > The GICv4.1 work [1] is going to prevent Linux booting on all current > > > > versions of Xen. While I can't confirm this is going to be merged in > > > > 5.5, I can tell you this will break. > > > > > > > > > > > > > > In any case, even if potential future Linux releases could have other > > > > > additional issues, I don't think it should change our current view on > > > > > this specific issue which affects 5.4, just released. > > > > > > > > The patch is definitely not as straightforward as you may think. > > > > Please refer to the discussion we had on the first version. I voiced > > > > concern about this approach and gave point what could go wrong with > > > happen. > > > > > > > > This patch may be better than the current state (i.e crashing), but > > > > this wasn't tested enough to confirm this is the correct things to do > > > > and no other bug will appear (I don't believe reading I*ACTIVER was > > > > ever tested before). > > > > > > > > It is an annoying bug, but this is only affecting 5.4 which has just > > > > been released. It feels to me this is a fairly risky choice to merge > > > > it qutie late in the release without a good graps of the problem (see > > > > above). > > > > > > > > So I would definitly, prefer if this patch is getting through backport > > > > once we get more testing. > > > > > > > > We can still document the bug in the release note and point people to > > > > the patch. > > > > > > > > Anyway, this is Juergen choice here. But at least now he has the full > > > > picture... > > > > > > > > Cheers, > > > > > > > > [1] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. > > > > > > > net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co > > > m%7Cdca > > > > > > > dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 > > > 5%7C0%7 > > > > > > > C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv > > > d73xb5 > > > > 2d6ERVQ%3Dreserved=0 > > > > > > > > > > Thanks, Julien, for sharing your opinion. > > > > > > With that statement I'd like to defer this patch to 4.14. > > > > But without this patch, 5.4 kernel will crash. So you prefer > > we develop the solution as Julien suggested for 4.13? > > I certainly won't take a patch for 4.13 when a maintainer of the > related code has reservations against it. > > I think the best thing to do is to develop a proper patch the > maintainers are happy with and don't try to force it into 4.13 now. > Such a patch can still be backported to 4.13 later. I chatted with Juergen and he explained to me something I didn't know before. The release manager can only *block* a patch from being committed, he/she cannot actually decide if the patch should be committed or not for a given release. He/she cannot overrule a maintainer either. In this case, Juergen cannot make the decision on whether the patch should go in 4.13 or not. Although I couldn't reproduce the problem on Xilinx boards, I have t
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On 27/11/2019 09:49, Peng Fan wrote: Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range On 27.11.19 10:31, Peng Fan wrote: Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range On 27.11.19 01:01, Julien Grall wrote: Hi, On 26/11/2019 23:17, Stefano Stabellini wrote: On Tue, 26 Nov 2019, Julien Grall wrote: Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? The GICv4.1 work [1] is going to prevent Linux booting on all current versions of Xen. While I can't confirm this is going to be merged in 5.5, I can tell you this will break. In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. The patch is definitely not as straightforward as you may think. Please refer to the discussion we had on the first version. I voiced concern about this approach and gave point what could go wrong with happen. This patch may be better than the current state (i.e crashing), but this wasn't tested enough to confirm this is the correct things to do and no other bug will appear (I don't believe reading I*ACTIVER was ever tested before). It is an annoying bug, but this is only affecting 5.4 which has just been released. It feels to me this is a fairly risky choice to merge it qutie late in the release without a good graps of the problem (see above). So I would definitly, prefer if this patch is getting through backport once we get more testing. We can still document the bug in the release note and point people to the patch. Anyway, this is Juergen choice here. But at least now he has the full picture... Cheers, [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co m%7Cdca dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 5%7C0%7 C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv d73xb5 2d6ERVQ%3Dreserved=0 Thanks, Julien, for sharing your opinion. With that statement I'd like to defer this patch to 4.14. But without this patch, 5.4 kernel will crash. So you prefer we develop the solution as Julien suggested for 4.13? Yes 5.4 will crash on Xen 4.13 (as any previous version of Xen). But I don't think this is right to push a patch late without a clear understanding of the problem. The argument so far has been we already implemented I*ACTIVER like that so it is fine to continue like that. However, I am not convinced the path has ever been exercised with older release of Linux and 5.4 will work as intended on Xen 4.13 with this patch. So I would recommend to read back my answer on v1 and trying to explain why this approach is acceptable to have. I certainly won't take a patch for 4.13 when a maintainer of the related code has reservations against it. I think the best thing to do is to develop a proper patch the maintainers are happy with and don't try to force it into 4.13 now. Such a patch can still be backported to 4.13 later. Ok. Julien, What's your suggestion to fix the issue? Do you have a rough idea? You can have a look at what the new vGIC is doing (see vgic/vgic-mmio.c). I don't know how feasible it is with the current vGIC. However, as I pointed out previously, this patch would be acceptable for the next version of Xen. But I don't think this is suitable for Xen 4.13 because we don't have enough data to lower the risk of this patch. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
> Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER > range > > On 27.11.19 10:31, Peng Fan wrote: > >> Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix > >> GICD_ISACTIVER range > >> > >> On 27.11.19 01:01, Julien Grall wrote: > >>> Hi, > >>> > >>> On 26/11/2019 23:17, Stefano Stabellini wrote: > >>>> On Tue, 26 Nov 2019, Julien Grall wrote: > >>>>> Hi, > >>>>> > >>>>> On 26/11/2019 20:43, Stefano Stabellini wrote: > >>>>>> + Juergen > >>>>>> > >>>>>> I missed that you weren't in CC to the original patch, sorry. > >>>>>> I think this patch should go in, as otherwise Linux 5.4 could run > >>>>>> into problems. It is also a pretty straightforward 4 lines patch. > >>>>> > >>>>> 5.5 (or 5.6) is not going to run on Xen for other reasons (still > >>>>> in the vGIC)... So I would not view this as critical. > >>>> > >>>> 5.5 is not out yet, in fact, the dev window has just opened. Isn't > >>>> your statement a bit premature? > >>> > >>> The GICv4.1 work [1] is going to prevent Linux booting on all > >>> current versions of Xen. While I can't confirm this is going to be > >>> merged in 5.5, I can tell you this will break. > >>> > >>>> > >>>> In any case, even if potential future Linux releases could have > >>>> other additional issues, I don't think it should change our current > >>>> view on this specific issue which affects 5.4, just released. > >>> > >>> The patch is definitely not as straightforward as you may think. > >>> Please refer to the discussion we had on the first version. I voiced > >>> concern about this approach and gave point what could go wrong with > >> happen. > >>> > >>> This patch may be better than the current state (i.e crashing), but > >>> this wasn't tested enough to confirm this is the correct things to > >>> do and no other bug will appear (I don't believe reading I*ACTIVER > >>> was ever tested before). > >>> > >>> It is an annoying bug, but this is only affecting 5.4 which has just > >>> been released. It feels to me this is a fairly risky choice to merge > >>> it qutie late in the release without a good graps of the problem (see > above). > >>> > >>> So I would definitly, prefer if this patch is getting through > >>> backport once we get more testing. > >>> > >>> We can still document the bug in the release note and point people > >>> to the patch. > >>> > >>> Anyway, this is Juergen choice here. But at least now he has the > >>> full picture... > >>> > >>> Cheers, > >>> > >>> [1] > >>> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. > >>> > >> > net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co > >> m%7Cdca > >>> > >> > dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 > >> 5%7C0%7 > >>> > >> > C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv > >> d73xb5 > >>> 2d6ERVQ%3Dreserved=0 > >>> > >> > >> Thanks, Julien, for sharing your opinion. > >> > >> With that statement I'd like to defer this patch to 4.14. > > > > But without this patch, 5.4 kernel will crash. So you prefer we > > develop the solution as Julien suggested for 4.13? > > I certainly won't take a patch for 4.13 when a maintainer of the related code > has reservations against it. > > I think the best thing to do is to develop a proper patch the maintainers are > happy with and don't try to force it into 4.13 now. > Such a patch can still be backported to 4.13 later. Ok. Julien, What's your suggestion to fix the issue? Do you have a rough idea? Thanks, Peng. > > > Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On 27.11.19 10:31, Peng Fan wrote: Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range On 27.11.19 01:01, Julien Grall wrote: Hi, On 26/11/2019 23:17, Stefano Stabellini wrote: On Tue, 26 Nov 2019, Julien Grall wrote: Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? The GICv4.1 work [1] is going to prevent Linux booting on all current versions of Xen. While I can't confirm this is going to be merged in 5.5, I can tell you this will break. In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. The patch is definitely not as straightforward as you may think. Please refer to the discussion we had on the first version. I voiced concern about this approach and gave point what could go wrong with happen. This patch may be better than the current state (i.e crashing), but this wasn't tested enough to confirm this is the correct things to do and no other bug will appear (I don't believe reading I*ACTIVER was ever tested before). It is an annoying bug, but this is only affecting 5.4 which has just been released. It feels to me this is a fairly risky choice to merge it qutie late in the release without a good graps of the problem (see above). So I would definitly, prefer if this patch is getting through backport once we get more testing. We can still document the bug in the release note and point people to the patch. Anyway, this is Juergen choice here. But at least now he has the full picture... Cheers, [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co m%7Cdca dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 5%7C0%7 C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv d73xb5 2d6ERVQ%3Dreserved=0 Thanks, Julien, for sharing your opinion. With that statement I'd like to defer this patch to 4.14. But without this patch, 5.4 kernel will crash. So you prefer we develop the solution as Julien suggested for 4.13? I certainly won't take a patch for 4.13 when a maintainer of the related code has reservations against it. I think the best thing to do is to develop a proper patch the maintainers are happy with and don't try to force it into 4.13 now. Such a patch can still be backported to 4.13 later. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
> Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER > range > > On 27.11.19 01:01, Julien Grall wrote: > > Hi, > > > > On 26/11/2019 23:17, Stefano Stabellini wrote: > >> On Tue, 26 Nov 2019, Julien Grall wrote: > >>> Hi, > >>> > >>> On 26/11/2019 20:43, Stefano Stabellini wrote: > >>>> + Juergen > >>>> > >>>> I missed that you weren't in CC to the original patch, sorry. > >>>> I think this patch should go in, as otherwise Linux 5.4 could run > >>>> into problems. It is also a pretty straightforward 4 lines patch. > >>> > >>> 5.5 (or 5.6) is not going to run on Xen for other reasons (still in > >>> the vGIC)... So I would not view this as critical. > >> > >> 5.5 is not out yet, in fact, the dev window has just opened. Isn't > >> your statement a bit premature? > > > > The GICv4.1 work [1] is going to prevent Linux booting on all current > > versions of Xen. While I can't confirm this is going to be merged in > > 5.5, I can tell you this will break. > > > >> > >> In any case, even if potential future Linux releases could have other > >> additional issues, I don't think it should change our current view on > >> this specific issue which affects 5.4, just released. > > > > The patch is definitely not as straightforward as you may think. > > Please refer to the discussion we had on the first version. I voiced > > concern about this approach and gave point what could go wrong with > happen. > > > > This patch may be better than the current state (i.e crashing), but > > this wasn't tested enough to confirm this is the correct things to do > > and no other bug will appear (I don't believe reading I*ACTIVER was > > ever tested before). > > > > It is an annoying bug, but this is only affecting 5.4 which has just > > been released. It feels to me this is a fairly risky choice to merge > > it qutie late in the release without a good graps of the problem (see > > above). > > > > So I would definitly, prefer if this patch is getting through backport > > once we get more testing. > > > > We can still document the bug in the release note and point people to > > the patch. > > > > Anyway, this is Juergen choice here. But at least now he has the full > > picture... > > > > Cheers, > > > > [1] > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. > > > net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co > m%7Cdca > > > dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7 > > > C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv > d73xb5 > > 2d6ERVQ%3Dreserved=0 > > > > Thanks, Julien, for sharing your opinion. > > With that statement I'd like to defer this patch to 4.14. But without this patch, 5.4 kernel will crash. So you prefer we develop the solution as Julien suggested for 4.13? Thanks, Peng. > > > Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On 27.11.19 01:01, Julien Grall wrote: Hi, On 26/11/2019 23:17, Stefano Stabellini wrote: On Tue, 26 Nov 2019, Julien Grall wrote: Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? The GICv4.1 work [1] is going to prevent Linux booting on all current versions of Xen. While I can't confirm this is going to be merged in 5.5, I can tell you this will break. In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. The patch is definitely not as straightforward as you may think. Please refer to the discussion we had on the first version. I voiced concern about this approach and gave point what could go wrong with happen. This patch may be better than the current state (i.e crashing), but this wasn't tested enough to confirm this is the correct things to do and no other bug will appear (I don't believe reading I*ACTIVER was ever tested before). It is an annoying bug, but this is only affecting 5.4 which has just been released. It feels to me this is a fairly risky choice to merge it qutie late in the release without a good graps of the problem (see above). So I would definitly, prefer if this patch is getting through backport once we get more testing. We can still document the bug in the release note and point people to the patch. Anyway, this is Juergen choice here. But at least now he has the full picture... Cheers, [1] https://lwn.net/Articles/800494/ Thanks, Julien, for sharing your opinion. With that statement I'd like to defer this patch to 4.14. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
Hi, On 26/11/2019 23:17, Stefano Stabellini wrote: On Tue, 26 Nov 2019, Julien Grall wrote: Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? The GICv4.1 work [1] is going to prevent Linux booting on all current versions of Xen. While I can't confirm this is going to be merged in 5.5, I can tell you this will break. In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. The patch is definitely not as straightforward as you may think. Please refer to the discussion we had on the first version. I voiced concern about this approach and gave point what could go wrong with happen. This patch may be better than the current state (i.e crashing), but this wasn't tested enough to confirm this is the correct things to do and no other bug will appear (I don't believe reading I*ACTIVER was ever tested before). It is an annoying bug, but this is only affecting 5.4 which has just been released. It feels to me this is a fairly risky choice to merge it qutie late in the release without a good graps of the problem (see above). So I would definitly, prefer if this patch is getting through backport once we get more testing. We can still document the bug in the release note and point people to the patch. Anyway, this is Juergen choice here. But at least now he has the full picture... Cheers, [1] https://lwn.net/Articles/800494/ -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On Tue, 26 Nov 2019, Julien Grall wrote: > Hi, > > On 26/11/2019 20:43, Stefano Stabellini wrote: > > + Juergen > > > > I missed that you weren't in CC to the original patch, sorry. > > I think this patch should go in, as otherwise Linux 5.4 could run into > > problems. It is also a pretty straightforward 4 lines patch. > > 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the > vGIC)... So I would not view this as critical. 5.5 is not out yet, in fact, the dev window has just opened. Isn't your statement a bit premature? In any case, even if potential future Linux releases could have other additional issues, I don't think it should change our current view on this specific issue which affects 5.4, just released. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
Hi, On 26/11/2019 20:43, Stefano Stabellini wrote: + Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. 5.5 (or 5.6) is not going to run on Xen for other reasons (still in the vGIC)... So I would not view this as critical. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
+ Juergen I missed that you weren't in CC to the original patch, sorry. I think this patch should go in, as otherwise Linux 5.4 could run into problems. It is also a pretty straightforward 4 lines patch. On Fri, 22 Nov 2019, Stefano Stabellini wrote: > On Fri, 22 Nov 2019, Peng Fan wrote: > > The end should be GICD_ISACTIVERN not GICD_ISACTIVER, > > and also print a warning for the unhandled read. > > > > Signed-off-by: Peng Fan > > --- > > > > V2: > > Add a warning message > > > > xen/arch/arm/vgic-v3.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > index 422b94f902..a15b9f6441 100644 > > --- a/xen/arch/arm/vgic-v3.c > > +++ b/xen/arch/arm/vgic-v3.c > > @@ -706,7 +706,10 @@ static int __vgic_v3_distr_common_mmio_read(const char > > *name, struct vcpu *v, > > goto read_as_zero; > > > > /* Read the active status of an IRQ via GICD/GICR is not supported */ > > -case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER): > > +case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): > > +printk(XENLOG_G_ERR "%pv: vGICD: unhandled read from > > ISACTIVER%d\n", > > + v, (reg - GICD_ISACTIVER) / 4); > > All the other similar printks that we have in vgic-v3.c don't have the > "/ 4", for instance: > > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): > if ( dabt.size != DABT_WORD ) goto bad_width; > printk(XENLOG_G_ERR >"%pv: %s: unhandled word write %#"PRIregister" to > ISACTIVER%d\n", >v, name, r, reg - GICD_ISACTIVER); > > However, reg reflects the address of the register, so actually, the > division by 4 looks correct if we want to get the index of the specific > register. Thanks for spotting this. We'll need to do a clean-up in the > file later. > > Reviewed-by: Stefano Stabellini > > > > > +goto read_as_zero; > > case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN): > > goto read_as_zero; ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On Fri, 22 Nov 2019, Peng Fan wrote: > The end should be GICD_ISACTIVERN not GICD_ISACTIVER, > and also print a warning for the unhandled read. > > Signed-off-by: Peng Fan > --- > > V2: > Add a warning message > > xen/arch/arm/vgic-v3.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 422b94f902..a15b9f6441 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -706,7 +706,10 @@ static int __vgic_v3_distr_common_mmio_read(const char > *name, struct vcpu *v, > goto read_as_zero; > > /* Read the active status of an IRQ via GICD/GICR is not supported */ > -case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER): > +case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): > +printk(XENLOG_G_ERR "%pv: vGICD: unhandled read from ISACTIVER%d\n", > + v, (reg - GICD_ISACTIVER) / 4); All the other similar printks that we have in vgic-v3.c don't have the "/ 4", for instance: case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): if ( dabt.size != DABT_WORD ) goto bad_width; printk(XENLOG_G_ERR "%pv: %s: unhandled word write %#"PRIregister" to ISACTIVER%d\n", v, name, r, reg - GICD_ISACTIVER); However, reg reflects the address of the register, so actually, the division by 4 looks correct if we want to get the index of the specific register. Thanks for spotting this. We'll need to do a clean-up in the file later. Reviewed-by: Stefano Stabellini > +goto read_as_zero; > case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN): > goto read_as_zero; > > -- > 2.16.4 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
