Re: [Qemu-devel] [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers

[Anthony Liguori <anthony@codemonkey.ws>]

Jan Kiszka wrote:
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index eed1f62..b7c8a2f 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -651,6 +651,20 @@ int cpu_get_pic_interrupt(CPUX86State *s);
>  /* MSDOS compatibility mode FPU exception support */
>  void cpu_set_ferr(CPUX86State *s);
>  
> +static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2)
> +{
> +    unsigned int limit;
> +    limit = (e1 & 0xffff) | (e2 & 0x000f0000);
> +    if (e2 & DESC_G_MASK)
> +        limit = (limit << 12) | 0xfff;
> +    return limit;
> +}
> +
> +static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2)
> +{
> +    return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
> +}
> +
>   

I like this patch but if you're going to export new x86 helper 
functions, please prefix them with cpu_x86. In this case, these helpers 
are awfully low level (they're taking a GDT entry split into two 32-bit 
words).  I would rather see an interface that took a CPUState and a 
segment register index.  In the very least, it won't be very obvious to 
anyone what this API expects to take so a comment would be required.

Regards,

Anthony Liguori