Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2024 18:21, Andrew Cooper wrote:
> On 06/03/2024 5:09 pm, Ross Lagerwall wrote:
>> On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 05.03.2024 13:11, Andrew Cooper wrote:
>>>> --- a/xen/include/xen/virtual_region.h
>>>> +++ b/xen/include/xen/virtual_region.h
>>>> @@ -16,6 +16,9 @@ struct virtual_region
>>>>      const void *text_start;                /* .text virtual address 
>>>> start. */
>>>>      const void *text_end;                  /* .text virtual address end. 
>>>> */
>>>>
>>>> +    const void *rodata_start;              /* .rodata virtual address 
>>>> start (optional). */
>>>> +    const void *rodata_end;                /* .rodata virtual address 
>>>> end. */
>>>> +
>>>>      /* If this is NULL the default lookup mechanism is used. */
>>>>      symbols_lookup_t *symbols_lookup;
>>> While perhaps the least bad one can do without quite a bit more churn,
>>> I'm still irritated by a virtual region (singular) suddenly covering
>>> two ranges of VA space. At the very least I think the description should
>>> say a word of justification in this regard. An alternative, after all,
>>> could have been for livepatch code to register separate regions for
>>> rodata (if present in a patch).
>>>
>>> A follow-on question then would be why ordinary data isn't reflected in
>>> a virtual region. Aiui that's just because livepatch presently has no
>>> need for it.
>>>
>>> Underlying question to both: Is the virtual region concept indeed meant
>>> to be fully tied to livepatch and its needs?
>>>
>> Virtual regions were introduced for live patching but I don't think it
>> is completely tied to live patching. It was introduced so that any code
>> can participate in symbol lookup, bug frame and exception table entry
>> search, rather than special casing "if livepatch" in many places.
>>
>> I agree that the virtual region concept is being abused here - it's just
>> being used as a convenient place to store rodata start/end and doesn't
>> really have much to do with the text start & end / bug frame / exception
>> table entry search that the virtual region is intended for.
>>
>> Maybe Andrew can explain why he used this approach?
> 
> I feel the simplicity and obviousness of patch 3 speaks for itself.
> 
> How do you propose fixing it differently.

I'm not opposed to doing it the way you do, but I think it then needs
clarifying (up front) what a virtual region really is. It looks to be
morphing into a module description instead ... One easy option might
be to have a comment next to the struct additions here making clear
that this is rather an abuse, but chosen to be this way to keep things
simple elsewhere.

Jan