Re: [Xen-devel] [PATCH v1 00/15] arm64: Mediate access to GICv3 sysregs at EL2

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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
> Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel