Re: [PATCH v2 3/3] x86/PVH: Support relocatable dom0 kernels

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 22:19, Jason Andryuk wrote:
> On 2024-03-14 10:19, Jan Beulich wrote:
>> On 14.03.2024 15:13, Jason Andryuk wrote:
>>> On 2024-03-14 09:21, Jan Beulich wrote:
>>>> On 13.03.2024 20:30, Jason Andryuk wrote:
>>>>> --- a/xen/include/public/elfnote.h
>>>>> +++ b/xen/include/public/elfnote.h
>>>>> @@ -194,6 +194,17 @@
>>>>>     */
>>>>>    #define XEN_ELFNOTE_PHYS32_ENTRY 18
>>>>>    
>>>>> +/*
>>>>> + * Physical loading constraints for PVH kernels
>>>>> + *
>>>>> + * Used to place constraints on the guest physical loading addresses and
>>>>> + * alignment for a PVH kernel.  This note's value is 3 64bit values in
>>>>> + * the following order: minimum, maximum and alignment.
>>>>
>>>> Along the lines of what I said on another sub-thread, I think at least
>>>> alignment wants to be optional here. Perhaps, with max going first, min
>>>> could also be optional.
>>>
>>> Interesting idea.
>>>
>>>> As indicated in different context by Roger, the values being uniformly
>>>> 64-bit ones also is questionable.
>>>>
>>>>> + * The presence of this note indicates the kernel is relocatable.
>>>>
>>>> I think it wants making explicit here that the act of relocating is still
>>>> left to the kernel.
>>>
>>> Ok.
>>>
>>> How is this for a new description?
>>>
>>> """
>>> Physical loading constraints for PVH kernels
>>>
>>> Used to place constraints on the guest physical loading addresses and
>>> alignment for a PVH kernel.
>>>
>>> The presence of this note indicates the kernel supports relocating itself.
>>>
>>> The note may include up to three 32bit values.
>>
>> I'm as unsure about always 32-bit as I am on it being uniformly 64-bit.
>> One question here is whether this note is intended to be x86-specific.
>>
>>>    - a maximum address for the entire image to be loaded below (default
>>> 0xfffffff)
>>
>> One f too few?
> 
> Whoops - yes.
> 
>>>    - a minimum address for the start of the image (default 0)
>>>    - a required start alignment (default 1)
> 
> Jan, in the discussion of patch 1, you wrote "Hmm, shouldn't the order 
> of attempts to figure the alignment be ELF note, ELF header, and then 
> 2Mb?"  My latest revision initializes phys_alignment to 1 and updates 
> that if PHYS32_RELOC specifies an alignment.  Do you still want these 
> other locations checked for alignment values?

I think it would be prudent to do so, yet at the same time I guess I won't
insist. Defaulting to 1, though, looks overly lax. In order for the
alignment value to be sensible to omit, the default needs to be sensible
(no lower than 4k, and quite likely better 2M).

Jan