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

[Marc Zyngier]

On 26/03/18 05:43, Manish Jaggi wrote:
> 
> 
> On 03/23/2018 12:28 PM, Julien Grall wrote:
>> (Sorry for the formatting)
>>
>> On 23 Mar 2018 14:46, "Manish Jaggi" > > 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.
> I gave a good reason, that it make sense to club two patches as we are
> not individually enabling g0/g1 for 30115 errata.
> There is no documented rule that code taken from linux has to be 1:1
> mapping with linux patch.

As the original author of these patches, I want to be able to easily
refer to the original design to see what has changed, and understand
why. I suspect I'm not the only one in this case. Given that there are
at most 4 people in this community who understand enough about the GIC

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

[Manish Jaggi]

On 03/23/2018 12:28 PM, Julien Grall wrote:

(Sorry for the formatting)

On 23 Mar 2018 14:46, "Manish Jaggi" > 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.
I gave a good reason, that it make sense to club two patches as we are 
not individually enabling g0/g1 for 30115 errata.
There is no documented rule that code taken from linux has to be 1:1 
mapping with linux patch.

When complete smmu.c is taken as one file the rule is not applied?



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

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

[Julien Grall]

(Sorry for the formatting)

On 23 Mar 2018 14:46, "Manish Jaggi"  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

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

[Manish Jaggi]

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

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

[Julien Grall]

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.


Cheers,

--
Julien Grall

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

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

[Manish Jaggi]

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.
For instance we are not providing any command line option to 
individually enable group1 grou0 traps.
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.

3)

Code organization:
1) Please drop __ from all functions

ok I will change.
2) All functions not exported *should* be static. At the same time 
you need to make sure that the series are bisectable. So you probably 
hook the file in the build system at the end rather than in #3.



ok.

Cheers,





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

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

[Julien Grall]

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.
	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.

3)

Code organization:
1) Please drop __ from all functions
	2) All functions not exported *should* be static. At the same time you 
need to make sure that the series are bisectable. So you probably hook 
the file in the build system at the end rather than in #3.


Cheers,


--
Julien Grall

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

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

[Julien Grall]

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..


"The current RFC patchset is a subset of [1], as it handling only Group 
1 traps as a PoC". This is clearly not a PoC anymore nor only handling 
Group 1.




Could you please review the other patches in the series, so that I can 
send v2.


It is in my queue of patch to review.

Cheers,

--
Julien Grall

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

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

[Manish Jaggi]

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.


Cheers,




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

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

[Julien Grall]

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.


Cheers,

--
Julien Grall

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