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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
