On Mon, 20 Aug 2012 09:52:56 +0200, Alexander Graf <agraf@xxxxxxx> wrote:
>
>
> Am 20.08.2012 um 06:57 schrieb Rusty Russell <rusty.russell@xxxxxxxxxx>:
>
> > On Thu, 16 Aug 2012 17:31:53 +0100, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
> >> Also QEMU's standard function for extracting bits from a bitfield is
> >> extract32(value, start, len), so just having the mask is entirely the
> >> wrong thing to feed into that. So these #defines are awkward both for
> >> constructing an index from a tuple and for extracting a tuple given an
> >> index.
> >>
> >> Basically QEMU is the primary userspace consumer of these constants;
> >> if you don't want to be helpful you might as well not define them
> >> in a public header file at all :-)
> >
> > To be precise, you'd prefer:
> >
> > #define KVM_ARM_MSR_COPROC_MASK_START 16
> > #define KVM_ARM_MSR_COPROC_MASK_LEN 16
> >
> > vs:
> > #define KVM_ARM_MSR_COPROC_MASK 0xFFFF0000
> >
> > What a huge readability lose. I can't describe how distasteful it is to
> > do this because QEMU would have to write a macro :(
>
> The usual way this is done in the kernel is
>
> #define xxx_SHIFT 16
> #define xxx_MASK (0xffff << xxx_SHIFT)
Actually, it's often not spelled out that way, but left as raw redundant
numbers, eg:
#define ATM_HDR_GFC_MASK 0xf0000000
#define ATM_HDR_GFC_SHIFT 28
#define ATM_HDR_VPI_MASK 0x0ff00000
#define ATM_HDR_VPI_SHIFT 20
#define ATM_HDR_VCI_MASK 0x000ffff0
#define ATM_HDR_VCI_SHIFT 4
#define ATM_HDR_PTI_MASK 0x0000000e
#define ATM_HDR_PTI_SHIFT 1
> Right? Would that work for you guys? It'd certainly be easier to read.
It's consistent existing practice. It still doesn't give qemu what they
want, which is the only reason I'm really changing it (plus, I had to
change the constants anyway to unify 32 and 64 bit coproc reg ids.).
Cheers,
Rusty.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
