xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien.grall@linaro.org>
To: Manish Jaggi <mjaggi@caviumnetworks.com>
Cc: sstabellini@kernel.org, marc.zyngier@arm.com,
	andre.przywara@arm.com, "Jaggi, Manish" <Manish.Jaggi@cavium.com>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v1 00/15] arm64: Mediate access to GICv3 sysregs at EL2
Date: Fri, 23 Mar 2018 06:58:00 +0000	[thread overview]
Message-ID: <CAEC2CxohC5AiMYAnoDu+=LLs-+QA-HrgpmE9w-fNH8G6qa9AYw@mail.gmail.com> (raw)
In-Reply-To: <48b5110b-1a07-a4fb-309c-b0fc4a334e09@caviumnetworks.com>


[-- Attachment #1.1: Type: text/plain, Size: 3389 bytes --]

(Sorry for the formatting)

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

[-- Attachment #1.2: Type: text/html, Size: 5182 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

  reply	other threads:[~2018-03-23  6:58 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-16 11:58 [PATCH v1 00/15] arm64: Mediate access to GICv3 sysregs at EL2 Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 01/15] arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family Manish Jaggi
2018-03-20  7:38   ` Julien Grall
2018-03-16 11:58 ` [PATCH v1 02/15] arm64: Add config for Cavium Thunder erratum 30115 Manish Jaggi
2018-03-20  7:43   ` Julien Grall
2018-03-21  5:06     ` Manish Jaggi
2018-03-21  7:49       ` Julien Grall
2018-03-16 11:58 ` [PATCH v1 03/15] arm: Placeholder for handling Group0/1 traps for Cavium Erratum 30115 Manish Jaggi
2018-03-20  8:08   ` Julien Grall
2018-03-16 11:58 ` [PATCH v1 04/15] arm64: vgic-v3: Add ICV_BPR1_EL1 handler Manish Jaggi
2018-03-21  8:11   ` Julien Grall
2018-03-26 13:11     ` Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 05/15] arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler Manish Jaggi
2018-03-21  8:38   ` Julien Grall
2018-03-26 13:09     ` Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 06/15] arm64: Add accessors for the ICH_APxRn_EL2 registers Manish Jaggi
2018-03-26 13:19   ` Manish Jaggi
2018-03-26 14:36     ` Marc Zyngier
2018-03-16 11:58 ` [PATCH v1 07/15] Expose ich_read/write_lr in vgic-v3-sr.c Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 08/15] arm64: Add ICV_IAR1_EL1 handler Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 09/15] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 10/15] arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 11/15] arm64: vgic-v3: Add ICV_BPR0_EL1 handler Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 12/15] arm64: vgic-v3: Add ICV_IGNREN0_EL1 handler Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 13/15] arm64: vgic-v3: Add misc Group-0 handlers Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 14/15] arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler Manish Jaggi
2018-03-16 11:58 ` [PATCH v1 15/15] Enable Group0/1 Traps by default for Cavium ThunderX1 Manish Jaggi
2018-03-20  7:46 ` [PATCH v1 00/15] arm64: Mediate access to GICv3 sysregs at EL2 Julien Grall
2018-03-21  4:58   ` Manish Jaggi
2018-03-21  5:02     ` Julien Grall
2018-03-21  8:45     ` Julien Grall
2018-03-21  9:38       ` Manish Jaggi
2018-03-21  9:56         ` Julien Grall
2018-03-23  6:42           ` Manish Jaggi
2018-03-23  6:58             ` Julien Grall [this message]
2018-03-26  4:43               ` Manish Jaggi
2018-03-26  8:24                 ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEC2CxohC5AiMYAnoDu+=LLs-+QA-HrgpmE9w-fNH8G6qa9AYw@mail.gmail.com' \
    --to=julien.grall@linaro.org \
    --cc=Manish.Jaggi@cavium.com \
    --cc=andre.przywara@arm.com \
    --cc=julien.grall@arm.com \
    --cc=marc.zyngier@arm.com \
    --cc=mjaggi@caviumnetworks.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).