Re: [PATCH v4] xen/riscv: PE/COFF image header for RISC-V target

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.07.2024 16:44, Milan Đokić wrote:
> On Mon, Jul 8, 2024 at 11:32 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 04.07.2024 19:21, Milan Đokić wrote:
>>> On Wed, Jul 3, 2024 at 5:55 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 03.07.2024 02:04, Milan Djokic wrote:
>>>>> +#ifdef CONFIG_RISCV_EFI
>>>>> +        /*
>>>>> +         * This instruction decodes to "MZ" ASCII required by UEFI.
>>>>> +         */
>>>>> +        c.li s4,-13
>>>>> +        c.j xen_start
>>>>> +#else
>>>>> +        /* jump to start kernel */
>>>>> +        jal xen_start
>>>>
>>>> JAL, not J? Why?
>>>>
>>> We use jal explicitly here to highlight that we want to occupy 32 bits
>>> in order to align with header format (and EFI case where we use two
>>> 16-bits instructions). Although it will also work by using "j"
>>> directly, it could be implicitly compressed to 16 bits (if C extension
>>> is available) which would make header layout less obvious imo.
>>
>> According to e.g. ...
>>
>>>>> +#endif
>>>>> +        .balign 8
>>>>
>>>> This won't do what you want unless "start" itself is also suitably aligned.
>>>> It'll be as long as it's first in the section, but better make such 
>>>> explicit.
>>>>
>>> I understand, we'll also explicitly align "start"
>>>
>>>>> +#ifdef CONFIG_RISCV_64
>>>>> +        /* Image load offset(2MB) from start of RAM */
>>>>> +        .quad 0x200000
>>>>> +#else
>>>>> +        /* Image load offset(4MB) from start of RAM */
>>>>> +        .quad 0x400000
>>>>> +#endif
>>
>> ... the #ifdef here, you aim at having code be suitable for RV32, too.
>> However, JAL has a compressed form there, so its use would make things
>> "less obvious" there as well. I'm inclined to say that since the
>> subsequent ".balign 8" adds padding NOPs anyway, there's no real
>> difference whether that adds 32 bits worth of NOPs or 48 bits. If you
>> really wanted to "hide" the difference, imo ".balign 4" would be the
>> way to go.
>>
>> In any event, if there would still be a reason to stick to JAL, you'd
>> want to name the reason(s) in a code comment.
>>
> No specific reason to use jal from a functional point of view, just
> aesthetic reasons as I mentioned in a previous comment. Also jal is
> used across head.S file, so we also keep some consistency in that
> manner. I just don't get the reason why "j" is more suitable as a
> pseudo instruction since it also comes down to jal as a base
> instruction.
> Of course, we can easily switch to "j" here, just want to know why we
> are doing so.

Consistency with the other path, which uses C.J. Plus avoiding questions
along the lines of mine: Why JAL when this isn't a function call, but a
plaing branch?

>>>>> --- /dev/null
>>>>> +++ b/xen/include/efi/pe.h
>>>>> [...]
>>> Regarding pe.h file content, we wanted to keep it as generic as
>>> possible (structures definition according to PE format which can be
>>> used for multiple architectures). Specifically for RISC-V as you
>>> noticed, we are not using lots of structures (data directories,
>>> relocation structures, certificates, etc.). Therefore, we can reduce
>>> it to only those used atm, but in that case we won't have a generic PE
>>> header definition anymore. Regarding structures which are already
>>> defined in common/efi/pe.c, meaning that with our change we have two
>>> versions of same structures, we can remove those, but in that case it
>>> could be confusing for someone who is trying to map fields from pe.h
>>> to actual image header in assembly code. Summary I would keep this
>>> header with its original content, but if you think that it contains
>>> too much overhead we can reduce it to relevant structures only.
>>
>> Actually I not only don't mind this header, but would consider it
>> superior to the present state of things. Just that then imo it would
>> want introducing in a separate commit, with suitable description /
>> justification. That could (should) then be followed by a patch using
>> this header's struct-s / definitions in pre-existing code, purging
>> any duplication from there.
>>
> So we should have one commit for pe.h only, a subsequent commit where
> x86 implementation is modified to use structures from pe.h and a third
> commit where we add EFI stub for RISC-V?

That's (in my view) the minimum level of splitting, yes.

> And we can submit the first two commits separately from the RISC-V EFI
> stub for which we'll wait on @Oleksii opinion on whether EFI
> application format is needed upstream?

Yes. I also wouldn't insist on the conversion part to be done right
away, just as long as it gets done not too far in the future.

However, since you say "whether": Imo the question isn't whether we need
that (the answer there is Yes as long as EFI exists for RISC-V), but
what form it should take. If we can properly link an EFI binary, I'd
generally consider this preferable over any hand crafting. Yet of course
there may be reasons why using the tool chain has to be ruled out.

Jan