Re: [PATCH v1 1/4] xen/riscv: add exception table support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2026 13:47, Oleksii Kurochko wrote:
> On 3/24/26 3:04 PM, Jan Beulich wrote:
>> On 13.03.2026 17:44, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/Makefile
>>> +++ b/xen/arch/riscv/Makefile
>>> @@ -3,6 +3,7 @@ obj-y += cpufeature.o
>>>   obj-y += domain.o
>>>   obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>>>   obj-y += entry.o
>>> +obj-$(CONFIG_HAS_EX_TABLE) += extables.o
>>
>> Simply obj-y please as long as the select is unconditional.
> 
> I see your point and at the moment there is no also other options how
> to handle case(s) for which exception table is introduced now. But if 
> potentially another mechanism will be introduced what will be the point 
> to have extable.o code in the final binary?

I'd like to suggest to keep things simple as long as they are simple. It
might be different if you firmly knew the other variant is going to be
needed pretty soon.

>>> +static bool ex_handler_fixup(const struct exception_table_entry *ex,
>>> +                                    struct cpu_user_regs *regs)
>>
>> Nit: Bad indentation.
>>
>>> +{
>>> +   regs->sepc = ex_fixup(ex);
>>> +
>>> +   return true;
>>
>> Nit: Bad use of hard tabs.
>>
>> And then - why the boolean return type, when this can't fail anyway?
> 
> As potentially we could have other handlers which might return not only 
> true, so it will be easier to handle return type inside fixup_exception().
> 
> But if you think there is no any sense to have for handlers the same 
> signature then I am also return void instead of bool for 
> ex_handler_fixup().

It's not like there's no sense in that at all, but again - let's keep things
simple as long as they are simple.

>>> +struct exception_table_entry {
>>> +   int32_t insn, fixup;
>>> +};
>>> +
>>> +extern struct exception_table_entry __start___ex_table[];
>>> +extern struct exception_table_entry __stop___ex_table[];
>>> +
>>> +#ifdef CONFIG_HAS_EX_TABLE
>>
>> Why, when this is a RISC-V specific header and HAS_EX_TABLE is selected
>> unconditionally?
> 
> To handle the potential in future case that CONFIG_HAS_EX_TABLE will 
> become conditional.
> I thought that it makes sense to be in sync with common/virtual_region.c 
> also uses ifdef around exception table related information.

But that's common code, which has to deal with mixed needs of the various
architectures.

>>> --- a/xen/include/xen/xen.lds.h
>>> +++ b/xen/include/xen/xen.lds.h
>>> @@ -219,4 +219,14 @@
>>>   #define VPCI_ARRAY
>>>   #endif
>>>   
>>> +#ifdef CONFIG_HAS_EX_TABLE
>>
>> No real need for this?
> 
> Here I can agree that there is not reason as if CONFIG_HAS_EX_TABLE is n
> then no one is expected to use exception table so the section is empty 
> and don't occupy any extra space in binary (except potentially some 
> space because of alignment).
> 
> 
>>
>>> +#define EX_TABLE                  \
>>> +        . = ALIGN(POINTER_ALIGN); \
>>
>> Strictly speaking the original 8 (in x86 code) as much as this is more
>> than we need - each element is a struct of 2 4-byte entities, after all.
> 
> For the  current struct - yes, we can do . = ALIGN(4) but if the 
> architecture will add uint64_t inside (or unsigned long) shouldn't we 
> then have ALIGN(POINTER_ALIGN)?

Along the lines of what I said above, here things want to be consistent.
The alignment effected should be possible to justify by just what is in
the tree, without resorting to any unknown future.

Jan