mbox series

[v2,0/3] xen/arm: Add emulation of Debug Data Transfer Registers

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

Message

Ayan Kumar Halder Dec. 18, 2023, 8:29 p.m. UTC
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

 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(-)

Comments

Luca Fancellu Dec. 20, 2023, 9:29 a.m. UTC | #1
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
> 
>
Luca Fancellu Dec. 20, 2023, 9:43 a.m. UTC | #2
> 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
>> 
>> 
>
Ayan Kumar Halder Dec. 20, 2023, 12:15 p.m. UTC | #3
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
>>>
>>>
Luca Fancellu Dec. 20, 2023, 12:27 p.m. UTC | #4
> 
> 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
Julien Grall Dec. 20, 2023, 1:16 p.m. UTC | #5
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,
Ayan Kumar Halder Dec. 20, 2023, 2:59 p.m. UTC | #6
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,
>