|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 5/7] xen/riscv: introduce and initialize SBI RFENCE extension
On Tue, 2024-08-13 at 11:34 +0200, Jan Beulich wrote:
> On 09.08.2024 18:19, Oleksii Kurochko wrote:
>
> > --- a/xen/arch/riscv/sbi.c
> > +++ b/xen/arch/riscv/sbi.c
> > @@ -7,11 +7,23 @@
> > * Modified by Bobby Eshleman (bobby.eshleman@xxxxxxxxx).
> > *
> > * Copyright (c) 2019 Western Digital Corporation or its
> > affiliates.
> > - * Copyright (c) 2021-2023 Vates SAS.
> > + * Copyright (c) 2021-2024 Vates SAS.
> > */
> >
> > +#include <xen/compiler.h>
> > +#include <xen/const.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/smp.h>
> > +
> > +#include <asm/processor.h>
> > #include <asm/sbi.h>
> >
> > +static unsigned long sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
> > +static unsigned long sbi_fw_id, sbi_fw_version;
>
> __ro_after_init for perhaps all three of them?
>
> Considering SBI_SPEC_VERSION_{MAJOR,MINOR}_MASK, at least the first
> of them also doesn't need to be unsigned long, but could be unsigned
> int?
It could be unsigned int, this part is declared as unsigned long in
Linux kernel so was taken as is. But based on the possible values for
these variables we can go with unsigned long.
>
> > @@ -38,7 +50,245 @@ struct sbiret sbi_ecall(unsigned long ext,
> > unsigned long fid,
> > return ret;
> > }
> >
> > +static int sbi_err_map_xen_errno(int err)
> > +{
> > + switch ( err )
> > + {
> > + case SBI_SUCCESS:
> > + return 0;
> > + case SBI_ERR_DENIED:
> > + return -EACCES;
> > + case SBI_ERR_INVALID_PARAM:
> > + return -EINVAL;
> > + case SBI_ERR_INVALID_ADDRESS:
> > + return -EFAULT;
> > + case SBI_ERR_NOT_SUPPORTED:
> > + return -EOPNOTSUPP;
> > + case SBI_ERR_FAILURE:
> > + fallthrough;
> > + default:
> > + return -ENOSYS;
> > + };
> > +}
> > +
> > void sbi_console_putchar(int ch)
> > {
> > sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> > }
> > +
> > +static unsigned long sbi_major_version(void)
> > +{
> > + return (sbi_spec_version >> SBI_SPEC_VERSION_MAJOR_SHIFT) &
> > + SBI_SPEC_VERSION_MAJOR_MASK;
> > +}
>
> Can you use MASK_EXTR() here, allowing to even drop the separate
> SBI_SPEC_VERSION_MAJOR_SHIFT?
I am not sure that:
(sbi_spec_version >> SBI_SPEC_VERSION_MAJOR_SHIFT) &
SBI_SPEC_VERSION_MAJOR_MASK == MASK_EXTR(sbi_spec_version,
SBI_SPEC_VERSION_MAJOR_MASK)
>
> > +static unsigned long sbi_minor_version(void)
> > +{
> > + return sbi_spec_version & SBI_SPEC_VERSION_MINOR_MASK;
> > +}
>
> For consistency here then, too.
Here we can use MASK_EXTR.
>
> > +static long sbi_ext_base_func(long fid)
> > +{
> > + struct sbiret ret;
> > +
> > + ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> > + if ( !ret.error )
> > + return ret.value;
> > + else
> > + return ret.error;
>
> With the folding of two distinct values, how is the caller going to
> tell failure from success?
By checking if the return value is < 0.
According to the SBI spec [
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/binary-encoding.adoc?plain=1#L32
] ret.error can be or 0 ( which means success ) or something < 0 if it
was a failure.
>
> > +}
> > +
> > +static int __sbi_rfence_v02_real(unsigned long fid,
>
> Are the double leading underscores really necessary here? (Same again
> further down.)
No at all, I'll drop it.
>
> > + unsigned long hmask, unsigned
> > long hbase,
> > + unsigned long start, unsigned
> > long size,
> > + unsigned long arg4)
> > +{
> > + struct sbiret ret = {0};
> > + int result = 0;
> > +
> > + switch ( fid )
> > + {
> > + case SBI_EXT_RFENCE_REMOTE_FENCE_I:
> > + ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> > + 0, 0, 0, 0);
> > + break;
> > + case SBI_EXT_RFENCE_REMOTE_SFENCE_VMA:
> > + ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> > + start, size, 0, 0);
> > + break;
> > + case SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID:
> > + ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> > + start, size, arg4, 0);
> > + break;
> > + case SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA:
> > + ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> > + start, size, 0, 0);
> > + break;
>
> How is e.g. this different from SBI_EXT_RFENCE_REMOTE_SFENCE_VMA
> handling? To ease recognition, I think you want to fold cases with
> identical handling.
Agree, it could be folded for some cases.
>
> > +int sbi_remote_sfence_vma(cpumask_t *cpu_mask,
> > + unsigned long start_addr,
> > + unsigned long size)
> > +{
> > + return __sbi_rfence(SBI_EXT_RFENCE_REMOTE_SFENCE_VMA,
> > + cpu_mask, start_addr, size, 0, 0);
>
> No check (not even an assertion) here for __sbi_rfence still being
> NULL?
I thought that it would be enough to have BUG_ON() in sbi_init() but
then probably would be better to update the message inside BUG_ON():
int __init sbi_init(void)
{
....
if ( !sbi_has_rfence() )
{
BUG_ON("At the moment flush_xen_tlb_range_va() uses SBI rfence
to "
"flush TLB for all CPUS!");
}
return 0;
}
>
> > +int __init sbi_init(void)
> > +{
> > + int ret;
> > +
> > + ret = sbi_get_spec_version();
> > + if ( ret > 0 )
> > + sbi_spec_version = ret;
>
> Truncation from long to int is not an issue here?
No, spec_version doesn't have such big numbers, the latest version is
v2.0.
>
> > + printk("SBI specification v%lu.%lu detected\n",
> > + sbi_major_version(), sbi_minor_version());
> > +
> > + if ( !sbi_spec_is_0_1() )
> > + {
> > + sbi_fw_id = sbi_get_firmware_id();
> > + sbi_fw_version = sbi_get_firmware_version();
>
> These cannot return errors?
At least, SBI specs don't tell that directly:
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-base.adoc?plain=1#L31
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |