(Sorry for the formatting) On 23 Mar 2018 14:46, "Manish Jaggi" <mja...@caviumnetworks.com> wrote:
On 03/21/2018 03:26 PM, Julien Grall wrote: > Hi Manish, > > On 03/21/2018 09:38 AM, Manish Jaggi wrote: > >> >> >> On 03/21/2018 02:15 PM, Julien Grall wrote: >> >>> >>> >>> On 03/21/2018 04:58 AM, Manish Jaggi wrote: >>> >>>> >>>> Hi Julien, >>>> >>>> On 03/20/2018 01:16 PM, Julien Grall wrote: >>>> >>>>> >>>>> >>>>> On 03/16/2018 11:58 AM, Manish Jaggi wrote: >>>>> >>>>>> This patchset is a Xen port of Marc's patchset. >>>>>> arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1] >>>>>> >>>>>> The current RFC patchset is a subset of [1], as it handleing only >>>>>> Group1 traps >>>>>> as a PoC. Most of the trap code is added in vsysreg.c. Trap handler >>>>>> function is kept >>>>>> independent of the usual guest trap handling code. >>>>>> Looking for feedback on this approach. >>>>>> >>>>> >>>>> This cover letter does not seem to match the series. Please update it >>>>> on every time you send a series. >>>>> >>>> %s/vsysreg.c/vgic-v3-sr.. >>>> >>>> Could you please review the other patches in the series, so that I can >>>> send v2. >>>> >>> >>> Here the major comments for the series (included patch not reviewed): >>> 1) You seem to miss some patches from Linux. I would like to >>> understand why they are not there. >>> >> if code is ported to xen, it is perfectly fine to take only relevant >> patches. >> > > It is usually expected from the contributor to have some sort of > explanation in the cover letter. In particular when you are based on a > series from Linux. > > Where I am more worried is there are patch on top in Linux, that you > didn't backport. So it would be really nice to understand why those patches > are not in Xen. > > A non-exhaustive list: > - KVM: arm64: Log an error if trapping a write-to-read-only GICv3 > access > - KVM: arm64: Log an error if trapping a read-from-write-only > GICv3 access > > > For instance we are not providing any command line option to individually >> enable group1 grou0 traps. >> > > I think the command line option could be useful for testing. Developer > don't necessarily have a Thunder-X in hand. > > 2) Strangely some commits does not match the Linux one either in order >>> and content (I am not speaking about the changes required by Xen). For >>> instance this is the case of patch #14 "arm64: vgic-v3: Add >>> ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should >>> follow the same. This help a lot for review. >>> >> Since we are not doing individually enable of group0/1, it doesnt make >> sense to have two set of patches for ICV_AP0 / ICV_AP1. So I merged it. >> > > Sorry, but it does not make sense. Looking at the series you pointed. I > don't see a patch just for ICV_AP0. Instead it is part of " KVM: arm64: > vgic-v3: Enable trapping of Group-0 system registers". You ported that > patch in Xen. > > If you see this patch, you will find this one specifically for ICV_AP1 https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html You didn't get my point... You still don't explain why you move the ICV_AP0 from "Enable trapping of Group-0 system registers" to that patch. If you take commit from Linux then don't move code between commit around unless there is a good reason. Please try to make the review a bit easier... Cheers,
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
