Re: [PATCH v2 5/5] xen/arm: Support ARM standard PV time for domains created via toolstack

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.07.2025 10:31, Koichiro Den wrote:
> On Wed, Jul 09, 2025 at 11:16:02AM +0200, Jan Beulich wrote:
>> On 09.07.2025 10:04, Koichiro Den wrote:
>>> On Mon, Jul 07, 2025 at 10:01:47AM +0200, Jan Beulich wrote:
>>>> On 05.07.2025 16:27, Koichiro Den wrote:
>>>>> +        mfn = virt_to_mfn(d->arch.pv_time_regions[idx]);
>>>>> +        t = p2m_ram_ro;
>>>>
>>>> Is this the correct type to use here? That is, do you really mean guest 
>>>> write
>>>> attempts to be silently dropped, rather than being reported to the guest 
>>>> as a
>>>> fault? Then again I can't see such behavior being implemented on Arm, 
>>>> despite
>>>> the comment on the enumerator saying so (likely inherited from x86).
>>>
>>> No I didn't intend the "silently drop" behavior. IIUC, we may as well
>>> correct the comment on the enum for Arm:
>>>
>>>     diff --git a/xen/arch/arm/include/asm/p2m.h 
>>> b/xen/arch/arm/include/asm/p2m.h
>>>     index 2d53bf9b6177..927c588dbcb0 100644
>>>     --- a/xen/arch/arm/include/asm/p2m.h
>>>     +++ b/xen/arch/arm/include/asm/p2m.h
>>>     @@ -123,7 +123,7 @@ struct p2m_domain {
>>>      typedef enum {
>>>          p2m_invalid = 0,    /* Nothing mapped here */
>>>          p2m_ram_rw,         /* Normal read/write guest RAM */
>>>     -    p2m_ram_ro,         /* Read-only; writes are silently dropped */
>>>     +    p2m_ram_ro,         /* Read-only */
>>
>> Don't know whether that's a good idea, as it'll diverge Arm from the same-
>> name P2M type that x86 has. (Arguably x86'es type may better be named
>> p2m_ram_write_ignore or some such.)
> 
> Thanks for sharing your thought. Incidentally, there seems to be the same
> comment in ppc's p2m.h as well.

That's likely (sorry to be blunt) due to blindly copying from Arm.

> I'm not sure at all but I'm guessing that
> "writes are silently dropped" line would make sense only when PoD

You can't silently drop writes in PoD mode, and PoD entries also have their
own type. All you can drop there are writes which store 0.

> and/or VM
> forking is to be supported by arm/ppc, and leaving it as it is for arm/ppc
> might be acceptable at the moment.

Leaving _behavior_ as is may indeed be acceptable. But naming and commentary
would still better change for things to be consistent across architectures,
rather than explicitly confusing people.

Jan