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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

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.

> 
> To start with, I'd recommend a head.S with _start: and an infinite loop,
> along with whatever makefile/kconfig infrastructure is required to get a
> build.
> 

Got it, sounds like a plan.

> 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.

> 
> Disabling grant tables in particular will work around the fact that the
> ARM snapshot you based your port on was pre-XSA-295, and the cmpxchg
> loop against guest memory in gnttab_clear_flags() is reintroducing a
> previously-fixed security vulnerability.

Duly noted, I'll fix that and look over some of those again and compare them to the
newer files, as some are definitely a few version bumps behind.

> 
> 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.

> As a couple of general points from our coding style, please avoid
> commented out code, and you are free to omit braces for single statement
> blocks.
> 
> ~Andrew


Thanks again,
-Bobby

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