Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node

From: Julien Grall <julien.grall@arm.com>
To: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Viktor Mitin <viktor.mitin.19@gmail.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Viktor Mitin <Viktor_Mitin@epam.com>
Subject: Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
Date: Wed, 31 Jul 2019 15:35:11 +0100	[thread overview]
Message-ID: <76a13c39-9b8b-eb3d-cf71-3603ac63c041@arm.com> (raw)
In-Reply-To: <875zniiao3.fsf@epam.com>

Hi Volodymyr,

On 31/07/2019 14:41, Volodymyr Babchuk wrote:
> Viktor Mitin writes:
>> On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com> wrote:
>>> So, previously this code copied "compatible" property from platform
>>> device tree. Please note, that theoretically it would be neither
>>> "arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
>>> two values. I'm not sure if this is right thing to do in the first
>>> place. Probably we need comment from Julien. But this change should be
>>> reflected in the commit message.
>>
>> Well, it is done, because Julien preferred domU variant as more simple one.
>> Actually I have checked that both variats works well, but kept domU case.
> My concern is that you are changing function behavior in
> non-backward compatible way. Yes, it is working on your platform. But
> what about others?

There are only official three compatible existing for the arch timer:
    - arm,armv7-timer
    - arm,armv8-timer
    - arm,cortex-a15-timer

The latter should always have also arm,armv7-timer. Any OS running on Xen should 
not assume that a specific property should be there. If it is not the case, then 
fix your OS.

I am also discarding any other property compatible as they are probably 
out-of-tree and therefore not supported.

For 64-bit domain, you can only ever run on Armv8 platform so there are no 
change here.

For 32-bit domain, you can run on either Armv8 and Armv7 platform. It is a grey 
area where you should pass a different property depending on the platform you 
are. Libxl is always passing armv7 so I would prefer to keep like that.

The main difference with this patch will be for 32-bit dom0. They will always 
see Armv7 compatible even when running on Armv8 platform.

Xen 32-bit on Armv8 platform is not supported. Running 32-bit dom0 on Xen 64-bit 
is very unlikely.

So I don't have any major concerns with this change. This has the advantage to 
uniformize the way arch timer is exposed to all the guests.

Cheers,

-- 
Julien Grall

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