| Message ID | 20231218202959.1390114-1-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xen/arm: Add emulation of Debug Data Transfer Registers | expand |
Hi Ayan, > On 18 Dec 2023, at 20:29, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote: > > Hi, > > Refer https://lore.kernel.org/all/alpine.DEB.2.22.394.2312071341540.1265976@ubuntu-linux-20-04-desktop/T/ > for the previous discussion on this issue. > > Also, the linux earlycon hvc driver has been fixed. > See https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=0ec058ece2f933aed66b76bd5cb9b5e6764853c3 > > Changes from v1:- > 1. Split the change across 3 patches as per the design consensus. > > Ayan Kumar Halder (3): > xen/arm: Add emulation of Debug Data Transfer Registers > xen: arm: Introduce CONFIG_PARTIAL_EMULATION > xen/arm: Introduce "partial-emulation" xen command line option I’m a bit puzzled about the order of these changes, wouldn’t be better to Introduce the partial emulation Kconfig and command line args in the same patch together with something that uses it? I think here you wanted to keep Michal patch untouched, but I feel that a correct split for this serie would be maybe introducing Kconfig, boot args and one of the arm64/arm32 emulation, and on a second patch the other one (Or vice-versa)? If there is something that I’m missing let me know. > > docs/misc/xen-command-line.pandoc | 7 +++++ > xen/arch/arm/Kconfig | 8 +++++ > xen/arch/arm/arm64/vsysreg.c | 32 ++++++++++++++++--- > xen/arch/arm/include/asm/arm64/hsr.h | 3 ++ > xen/arch/arm/include/asm/cpregs.h | 2 ++ > xen/arch/arm/include/asm/regs.h | 6 ++++ > xen/arch/arm/include/asm/traps.h | 4 +++ > xen/arch/arm/traps.c | 21 +++++++++++++ > xen/arch/arm/vcpreg.c | 47 +++++++++++++++++++++++----- > 9 files changed, 117 insertions(+), 13 deletions(-) > > -- > 2.25.1 > >
> On 20 Dec 2023, at 09:29, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > Hi Ayan, > >> On 18 Dec 2023, at 20:29, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote: >> >> Hi, >> >> Refer https://lore.kernel.org/all/alpine.DEB.2.22.394.2312071341540.1265976@ubuntu-linux-20-04-desktop/T/ >> for the previous discussion on this issue. >> >> Also, the linux earlycon hvc driver has been fixed. >> See https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=0ec058ece2f933aed66b76bd5cb9b5e6764853c3 >> >> Changes from v1:- >> 1. Split the change across 3 patches as per the design consensus. >> >> Ayan Kumar Halder (3): >> xen/arm: Add emulation of Debug Data Transfer Registers >> xen: arm: Introduce CONFIG_PARTIAL_EMULATION >> xen/arm: Introduce "partial-emulation" xen command line option > > I’m a bit puzzled about the order of these changes, wouldn’t be better to > Introduce the partial emulation Kconfig and command line args in the same > patch together with something that uses it? > > I think here you wanted to keep Michal patch untouched, but I feel that a > correct split for this serie would be maybe introducing Kconfig, boot args and > one of the arm64/arm32 emulation, and on a second patch the other one > (Or vice-versa)? ^^^^^^^-----> for the vice-versa, I meant 1st patch Arm64, 2nd Arm32 or vice-versa. > > If there is something that I’m missing let me know. > >> >> docs/misc/xen-command-line.pandoc | 7 +++++ >> xen/arch/arm/Kconfig | 8 +++++ >> xen/arch/arm/arm64/vsysreg.c | 32 ++++++++++++++++--- >> xen/arch/arm/include/asm/arm64/hsr.h | 3 ++ >> xen/arch/arm/include/asm/cpregs.h | 2 ++ >> xen/arch/arm/include/asm/regs.h | 6 ++++ >> xen/arch/arm/include/asm/traps.h | 4 +++ >> xen/arch/arm/traps.c | 21 +++++++++++++ >> xen/arch/arm/vcpreg.c | 47 +++++++++++++++++++++++----- >> 9 files changed, 117 insertions(+), 13 deletions(-) >> >> -- >> 2.25.1 >> >> >
On 20/12/2023 09:43, Luca Fancellu wrote: > >> On 20 Dec 2023, at 09:29, Luca Fancellu <Luca.Fancellu@arm.com> wrote: >> >> Hi Ayan, Hi Luca, Thanks for looking into this. >> >>> On 18 Dec 2023, at 20:29, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote: >>> >>> Hi, >>> >>> Refer https://lore.kernel.org/all/alpine.DEB.2.22.394.2312071341540.1265976@ubuntu-linux-20-04-desktop/T/ >>> for the previous discussion on this issue. >>> >>> Also, the linux earlycon hvc driver has been fixed. >>> See https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=0ec058ece2f933aed66b76bd5cb9b5e6764853c3 >>> >>> Changes from v1:- >>> 1. Split the change across 3 patches as per the design consensus. >>> >>> Ayan Kumar Halder (3): >>> xen/arm: Add emulation of Debug Data Transfer Registers >>> xen: arm: Introduce CONFIG_PARTIAL_EMULATION >>> xen/arm: Introduce "partial-emulation" xen command line option >> I’m a bit puzzled about the order of these changes, wouldn’t be better to >> Introduce the partial emulation Kconfig and command line args in the same >> patch together with something that uses it? You have read my mind. :) I wan't sure about how to do the split... >> >> I think here you wanted to keep Michal patch untouched, but I feel that a >> correct split for this serie would be maybe introducing Kconfig, boot args and >> one of the arm64/arm32 emulation, and on a second patch the other one >> (Or vice-versa)? > ^^^^^^^-----> for the vice-versa, I meant 1st patch Arm64, 2nd Arm32 > or vice-versa. I wanted to keep Michal's change for the arm64 as it is. And introduce Kconfig and command line in separate patches. But I agree, it does not look clean. I am fine to follow your suggestion ie (Option 1) Patch 1 ---> arm64 changes + Kconfig + cmd_line (authored by Michal) Patch 2 ---> arm32 changes (which will use the Kconfig and cmd_line introduced before) Alternatively, I am thinking like this (Option 2) Patch 1 --> arm64 changes (authored by Michal) Patch 2 --> arm32 changes Patch 3 ---> Kconfig + cmd_line (which will touch patch 1 and 2). Let me know what you (+ Stefano, Julien, Bertrand) think about the split. I don't have a strong preference on how the split should be done. - Ayan >> If there is something that I’m missing let me know. >> >>> docs/misc/xen-command-line.pandoc | 7 +++++ >>> xen/arch/arm/Kconfig | 8 +++++ >>> xen/arch/arm/arm64/vsysreg.c | 32 ++++++++++++++++--- >>> xen/arch/arm/include/asm/arm64/hsr.h | 3 ++ >>> xen/arch/arm/include/asm/cpregs.h | 2 ++ >>> xen/arch/arm/include/asm/regs.h | 6 ++++ >>> xen/arch/arm/include/asm/traps.h | 4 +++ >>> xen/arch/arm/traps.c | 21 +++++++++++++ >>> xen/arch/arm/vcpreg.c | 47 +++++++++++++++++++++++----- >>> 9 files changed, 117 insertions(+), 13 deletions(-) >>> >>> -- >>> 2.25.1 >>> >>>
> > I wanted to keep Michal's change for the arm64 as it is. And introduce Kconfig and command line in separate patches. > > But I agree, it does not look clean. > > > I am fine to follow your suggestion ie (Option 1) > > Patch 1 ---> arm64 changes + Kconfig + cmd_line (authored by Michal) > > Patch 2 ---> arm32 changes (which will use the Kconfig and cmd_line introduced before) > > > Alternatively, I am thinking like this (Option 2) > > Patch 1 --> arm64 changes (authored by Michal) > > Patch 2 --> arm32 changes > > Patch 3 ---> Kconfig + cmd_line (which will touch patch 1 and 2). > > Let me know what you (+ Stefano, Julien, Bertrand) think about the split. > > I don't have a strong preference on how the split should be done. The maintainers will decide, but I feel that with option 2, there will be, in the tree, a state where the design decision won’t be fulfilled, so with patch 1 and 2 we will have partial emulation unconditionally. Instead with option 1 we will have always a proper state, which will be arm64 conditional partial emulation with patch 1 and arm32 conditional partial emulation with patch 2. Maybe maintainers will decide if that matters or not. Cheers, Luca
Hi, On 20/12/2023 12:15, Ayan Kumar Halder wrote: > > On 20/12/2023 09:43, Luca Fancellu wrote: >> >>> On 20 Dec 2023, at 09:29, Luca Fancellu <Luca.Fancellu@arm.com> wrote: >>> >>> Hi Ayan, > > Hi Luca, > > Thanks for looking into this. > >>> >>>> On 18 Dec 2023, at 20:29, Ayan Kumar Halder >>>> <ayan.kumar.halder@amd.com> wrote: >>>> >>>> Hi, >>>> >>>> Refer >>>> https://lore.kernel.org/all/alpine.DEB.2.22.394.2312071341540.1265976@ubuntu-linux-20-04-desktop/T/ >>>> for the previous discussion on this issue. >>>> >>>> Also, the linux earlycon hvc driver has been fixed. >>>> See >>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=0ec058ece2f933aed66b76bd5cb9b5e6764853c3 >>>> >>>> Changes from v1:- >>>> 1. Split the change across 3 patches as per the design consensus. >>>> >>>> Ayan Kumar Halder (3): >>>> xen/arm: Add emulation of Debug Data Transfer Registers >>>> xen: arm: Introduce CONFIG_PARTIAL_EMULATION >>>> xen/arm: Introduce "partial-emulation" xen command line option >>> I’m a bit puzzled about the order of these changes, wouldn’t be >>> better to >>> Introduce the partial emulation Kconfig and command line args in the >>> same >>> patch together with something that uses it? > > You have read my mind. :) I wan't sure about how to do the split... > >>> >>> I think here you wanted to keep Michal patch untouched, but I feel >>> that a >>> correct split for this serie would be maybe introducing Kconfig, boot >>> args and >>> one of the arm64/arm32 emulation, and on a second patch the other one >>> (Or vice-versa)? >> ^^^^^^^-----> for the vice-versa, I meant 1st patch Arm64, 2nd Arm32 >> or vice-versa. > > I wanted to keep Michal's change for the arm64 as it is. And introduce > Kconfig and command line in separate patches. I agree and the Kconfig/command line should be a separate patches. However, I think it should be introduced first rather than last (I don't want the code to be always enabled even temporiraly). So my preferred option would be... > > But I agree, it does not look clean. > > > I am fine to follow your suggestion ie (Option 1) > > Patch 1 ---> arm64 changes + Kconfig + cmd_line (authored by Michal) > > Patch 2 ---> arm32 changes (which will use the Kconfig and cmd_line > introduced before) > > > Alternatively, I am thinking like this (Option 2) > > Patch 1 --> arm64 changes (authored by Michal) > > Patch 2 --> arm32 changes > > Patch 3 ---> Kconfig + cmd_line (which will touch patch 1 and 2). Option 3 Patch 1 -> Kconfig + cmd_line Patch 2 -> Arm64 changes Patch 3 -> Arm32 changes I could also settle with option 1. Cheers,
On 20/12/2023 13:16, Julien Grall wrote: > Hi, Hi Julien/Luca, > > On 20/12/2023 12:15, Ayan Kumar Halder wrote: >> >> On 20/12/2023 09:43, Luca Fancellu wrote: >>> >>>> On 20 Dec 2023, at 09:29, Luca Fancellu <Luca.Fancellu@arm.com> wrote: >>>> >>>> Hi Ayan, >> >> Hi Luca, >> >> Thanks for looking into this. >> >>>> >>>>> On 18 Dec 2023, at 20:29, Ayan Kumar Halder >>>>> <ayan.kumar.halder@amd.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> Refer >>>>> https://lore.kernel.org/all/alpine.DEB.2.22.394.2312071341540.1265976@ubuntu-linux-20-04-desktop/T/ >>>>> for the previous discussion on this issue. >>>>> >>>>> Also, the linux earlycon hvc driver has been fixed. >>>>> See >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=0ec058ece2f933aed66b76bd5cb9b5e6764853c3 >>>>> >>>>> Changes from v1:- >>>>> 1. Split the change across 3 patches as per the design consensus. >>>>> >>>>> Ayan Kumar Halder (3): >>>>> xen/arm: Add emulation of Debug Data Transfer Registers >>>>> xen: arm: Introduce CONFIG_PARTIAL_EMULATION >>>>> xen/arm: Introduce "partial-emulation" xen command line option >>>> I’m a bit puzzled about the order of these changes, wouldn’t be >>>> better to >>>> Introduce the partial emulation Kconfig and command line args in >>>> the same >>>> patch together with something that uses it? >> >> You have read my mind. :) I wan't sure about how to do the split... >> >>>> >>>> I think here you wanted to keep Michal patch untouched, but I feel >>>> that a >>>> correct split for this serie would be maybe introducing Kconfig, >>>> boot args and >>>> one of the arm64/arm32 emulation, and on a second patch the other one >>>> (Or vice-versa)? >>> ^^^^^^^-----> for the vice-versa, I meant 1st patch Arm64, 2nd Arm32 >>> or vice-versa. >> >> I wanted to keep Michal's change for the arm64 as it is. And >> introduce Kconfig and command line in separate patches. > > I agree and the Kconfig/command line should be a separate patches. > However, I think it should be introduced first rather than last (I > don't want the code to be always enabled even temporiraly). So my > preferred option would be... > >> >> But I agree, it does not look clean. >> >> >> I am fine to follow your suggestion ie (Option 1) >> >> Patch 1 ---> arm64 changes + Kconfig + cmd_line (authored by Michal) >> >> Patch 2 ---> arm32 changes (which will use the Kconfig and cmd_line >> introduced before) >> >> >> Alternatively, I am thinking like this (Option 2) >> >> Patch 1 --> arm64 changes (authored by Michal) >> >> Patch 2 --> arm32 changes >> >> Patch 3 ---> Kconfig + cmd_line (which will touch patch 1 and 2). > > > Option 3 > > Patch 1 -> Kconfig + cmd_line > Patch 2 -> Arm64 changes > Patch 3 -> Arm32 changes I like this option and will go with this. If you can review the current patches, that will be great. I will incorporate your feedback while sending v3. - Ayan > > I could also settle with option 1. > > Cheers, >