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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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@xxxxxxx> wrote:
> > > >
> > > > Hi Ayan,
> > Hi Luca,
> >
> > Thanks for looking into this.
> >
> > > > > On 18 Dec 2023, at 20:29, Ayan Kumar Halder 
> > > > > <ayan.kumar.halder@xxxxxxx> 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,