Re: [PATCH v3 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/02/2023 12:00, Oleksii wrote:
> On Tue, 2023-02-07 at 16:07 +0100, Jan Beulich wrote:
> > On 07.02.2023 15:46, Oleksii Kurochko wrote:
> > > --- /dev/null
> > > +++ b/xen/arch/riscv/include/asm/bug.h
> > > @@ -0,0 +1,38 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2012 Regents of the University of California
> > > + * Copyright (C) 2021-2023 Vates
> > > + *
> > > + */
> > > +#ifndef _ASM_RISCV_BUG_H
> > > +#define _ASM_RISCV_BUG_H
> > > +
> > > +#include <xen/types.h>
> > > +
> > > +#ifndef __ASSEMBLY__
> > > +
> > > +#define BUG_FN_REG t0
> > > +
> > > +#define BUG_INSTR "ebreak"
> > > +
> > > +#define INSN_LENGTH_MASK        _UL(0x3)
> > > +#define INSN_LENGTH_32          _UL(0x3)
> > I assume these are deliberately over-simplified (not accounting for
> > wider than 32-bit insns in any way)?
> The base instruction set has a fixed length of 32-bit naturally aligned
> instructions.
>
> There are extensions of variable length ( where each instruction can be
> any number of 16-bit parcels in length ) but they aren't used in Xen
> and Linux kernel ( where these definitions were taken from ).
>
> Compressed ISA is used now where the instruction length is 16 bit  and
> 'ebreak' instruction, in this case, can be either 16 or 32 bit (
> depending on if compressed ISA is used or not )
> > > +#define BUG_INSN_32             _UL(0x00100073) /* ebreak */
> > > +#define BUG_INSN_16             _UL(0x9002)     /* c.ebreak */
> > > +#define COMPRESSED_INSN_MASK    _UL(0xffff)
> > > +
> > > +#define GET_INSN_LENGTH(insn)                               \
> > > +({                                                          \
> > > +    unsigned long len;                                      \
> > > +    len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
> > > +        4UL : 2UL;                                          \
> > > +    len;                                                    \
> > Any reason for the use of "unsigned long" (not "unsigned int") here?
> There is no specific reason (at least I don't see it now). It looks
> like it can be used here even smaller type than 'unsigned int' as len,
> in current case, can be either 4 or 2.
>
> > > +})
> > > +
> > > +/* These are defined by the architecture */
> > > +int is_valid_bugaddr(uint32_t addr);
> > A function by this name very likely wants to return bool. Also -
> > uint32_t (not "unsigned long") for an address?
> It should be unsigned long.
In other part of the code, you are using vaddr_t/paddr_t to describe an 
address. It would be best to use one of those (I think vaddr_t) so it is 
clear what kind of address it is supposed to take.
Cheers,

--
Julien Grall