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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/03/2024 7:39 am, Jan Beulich wrote:
> 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.

The thing called virtual_region already describes 6 ranges, and I'm
adding a 7th.

It has been a module-ish description right from the very outset.  I
don't think it is fair to describe this as an abuse at all.

Is this going to satisfy the outstanding concerns?

diff --git a/xen/include/xen/virtual_region.h
b/xen/include/xen/virtual_region.h
index d05362071135..9d150beb8a87 100644
--- a/xen/include/xen/virtual_region.h
+++ b/xen/include/xen/virtual_region.h
@@ -9,6 +9,12 @@
 #include <xen/list.h>
 #include <xen/symbols.h>
 
+/*
+ * Despite it's name, this is module(ish) description.
+ *
+ * There's one region for .text/etc, one region for .init during boot only,
+ * and one region per livepatch.
+ */
 struct virtual_region
 {
     struct list_head list;

~Andrew