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

From: Manish Jaggi <mjaggi@caviumnetworks.com>
To: Julien Grall <julien.grall@arm.com>,
	Manish Jaggi <manish.jaggi@cavium.com>,
	xen-devel@lists.xenproject.org, sstabellini@kernel.org,
	marc.zyngier@arm.com, andre.przywara@arm.com
Subject: Re: [PATCH v1 00/15] arm64: Mediate access to GICv3 sysregs at EL2
Date: Fri, 23 Mar 2018 12:12:15 +0530	[thread overview]
Message-ID: <48b5110b-1a07-a4fb-309c-b0fc4a334e09@caviumnetworks.com> (raw)
In-Reply-To: <ca515ba7-c53f-62e9-a495-8c330c36c089@arm.com>


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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel