Re: [Qemu-devel] [PATCH 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Peter Maydell]

On 24 October 2018 at 12:37, Richard Henderson
<address@hidden> wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/kvm64.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 5de8ff0ac5..6ed80eadc2 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -443,17 +443,41 @@ static inline void unset_feature(uint64_t *features, 
> int feature)
>      *features &= ~(1ULL << feature);
>  }
>
> +static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
> +{
> +    uint64_t ret;
> +    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)&ret };
> +    int err;
> +
> +    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U64);
> +    err = ioctl(fd, KVM_GET_ONE_REG, &idreg);
> +    if (err < 0) {
> +        return -1;
> +    }
> +    assert(ret <= UINT32_MAX);

In AArch64, there isn't really any such thing as a 32 bit sysreg.
Where the Arm ARM refers to 32 bit system registers this is just
a shorthand for "64-bit register where the top 32 bits are currently
RES0". This assert() would result in QEMU crashing if we ever ran
it on a host for some hypothetical future architecture which
assigned meanings to some of the high bits. (Granted, this is
unlikely for the specific case of the ID registers which track
the AArch32 ID register values.)

I think I would prefer it if we expanded the id_isar* fields
in the ARMISARegisters struct to uint64_t. If you dislike
that, I think we should make this code fail a bit more gracefully
in the presence of an unexpected extension into the high bits
of these registers. Or just ignore the high bits, since we're
effectively trusting that future architecture versions use
a compatible meaning for these registers anyway.

> +    *pret = ret;
> +    return 0;
> +}
> +
> +static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
> +{
> +    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)pret };
> +
> +    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U64);
> +    return ioctl(fd, KVM_GET_ONE_REG, &idreg);
> +}
>

thanks
-- PMM