Re: [PATCH] riscv: Add initial Xen guest support for RISC-V

[Andrew Jones <ajones@xxxxxxxxxxxxxxxx>]

On Wed, Jan 15, 2025 at 08:04:05PM +0100, Milan Đokić wrote:
> Hello Andrew,
> 
> On Tue, Jan 14, 2025 at 7:18 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Jan 14, 2025 at 05:09:36PM +0100, Milan Djokic wrote:
...
> > > +++ b/arch/riscv/xen/hypercall.S
> > > @@ -0,0 +1,71 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#include <linux/linkage.h>
> > > +#include <asm/assembler.h>
> > > +#include <xen/interface/xen.h>
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_xen_version);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_console_io);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_sched_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_hvm_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op_raw);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_multicall);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist);
> > > +EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op);
> > > +EXPORT_SYMBOL_GPL(privcmd_call);
> > > +#define SBI_ECALL 0xE
> >
> > Shouldn't this be 0xA000007, i.e. the SBI firmware specific extension
> > for Xen. Otherwise why refer to SBI? Note, '0xE' is an invalid, legacy
> > extension ID in SBI.
> >
> Hypercall is triggered through SBI and we defined 0xE just as an
> SBI_ECALL ID on Xen side for hypercall handling (among other operation
> IDs), so we're not referring to some standard /legacy ID here, just
> utilizing SBI for hypercall handling.

If the SBI specified EIDs and binary encoding aren't used, then the
hypercalls aren't "triggered through SBI", Xen is just doing its own
thing on an ecall. Xen doesn't have to implement SBI at all, but if
it wants to provide SBI services, as well as its own hypercalls, then
the hypercalls should be encoded in the same way as SBI functions and
an EID allowed by the SBI specification for hypervisor-specific
functions should be used. For Xen, that EID is already specified and
it's 0xA000007.

> Is this specific ID (0xE) not allowed to be used on the kernel side
> for some reason? If that is the case, we can use any other ID,
> including the one which you suggested.
>

Linux can use any ID, and Xen can decide what that ID is, but, if Xen
wants to implement SBI, then Xen can't use 0xE, because that EID is
not allowed by the spec for the purpose of hypercalls.

Thanks,
drew