Re: [Xen-devel] [RFC XEN PATCH 00/23] xen: beginning support for RISC-V

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/01/2020 03:26, Bobby Eshleman wrote:
> On Fri, Jan 24, 2020 at 01:41:38PM +0000, Andrew Cooper wrote:
>> On 22/01/2020 01:58, Bobby Eshleman wrote:
>>> Currently, this patchset really only sets up virtual memory for Xen and
>>> initializes UART to enable print output.  None of RISC-V's
>>> virtualization support has been implemented yet, although that is the
>>> next road to start going down.  Many functions only contain dummy
>>> implementations.  Many shortcuts have been taken and TODO's have been
>>> left accordingly.  It is very, very rough.  Be forewarned: you are quite
>>> likely to see some ungainly code here (despite my efforts to clean it up
>>> before sending this patchset out).  My intent with this RFC is to align
>>> early and gauge interest, as opposed to presenting a totally complete
>>> patchset.
>> I've taken a very quick look over the series.
>>
>> Normally, we require that all patches be bisectable, and this series is
>> not because of the Makefile changes in patch 3 specifying object files
>> which are introduced in the following 20 patches.  Of course,
>> introducing a brand new architecture is a special case, but the
>> suggested plan of getting a "minimal build" together will help keep the
>> second phase of "making it boot".
> That totally makes sense.

I see I trailed off in the middle of that final sentence, but it seems I
did get my point across.

>> Within that first phase however, there are some obvious tweaks we should
>> make to common code.  For one, the debugger_trap() infrastructure really
>> is x86-specific (and I haven't seen it used in a decade) so should have
>> its ARM stubs dropped so as not to be a burden on other architectures.
>>
>> There are other aspects (the atomic_t infrastructure) where x86 and ARM
>> already have basically identical copies of the header file, and you've
>> taken a 3rd copy.
>>
>> Other areas where you can reduce the minimum build would be to put some
>> defaults into the defconfig, such as disabling grant tables and mem
>> access.  There are almost certainly others which will help, so have a
>> search around menuconfig.
> Taking note of these, I can definitely do that.

To fore-warn you, there probably will be more things like this which
become apparent on further review.

While I don't expect you to do all the cleanup (some will almost
certainly be quicker for others to do), I would like to take the
opportunity to do the obvious bits of cleanup, given the rare
opportunity of seeing the minimum viable set of things to make a new
arch compile.

>> Some ARM-isms you've inherited would be much better if you didn't.  In
>> particular, I *really* hope RISC-V hasn't made the same backwards naming
>> bug in their pagetable levels which results in {second,first,zeroth}_*
>> et al.  In x86, we purposefully chose not to follow Intel's naming, and
>> instead went with L1, L2, L3, and for 64bit L4.
> The RISC-V spec does indeed describe the table level numbers in reverse
> order: L2 points to L1 points to L0 (for 39bit addresses).
>
> For SV48 48bit addresses, it's L3, L2, L1, to L0.
>
> That said, in the spec there is no direct mention of LX for levels, and
> instead these are referred to by the section from the "virtual page
> number" that they are referred by, (ie VPN[3], VPN[2], etc...).
>
> I definitely would not be strongly opposed to this change for a form
> that reads better.

This looks to be the same as what we've chosen to do in x86, give or
take a fencepost.

I'd recommend using L{3..0} for similarity with the spec (you surely
don't want an off-by-one difference between code in Xen and the spec). 
Whatever you end up choosing, put a description at the top of mm.h (or
wherever appropriate) which briefly introduces the terminology in Xen,
and cross references any differences with the spec.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel