[PATCH 3/6] i386: global descriptor table manipulation functions

Jan Dolezal dolezj21 at fel.cvut.cz
Tue Nov 18 15:46:50 UTC 2014


On 12.11.2014 16:42, Gedare Bloom wrote:
> On Wed, Nov 12, 2014 at 10:07 AM, Jan Dolezal <dolezj21 at fel.cvut.cz> wrote:
>> ---
>>   c/src/lib/libbsp/i386/shared/irq/idt.c | 147 +++++++++++++++++++++++++--------
>>   c/src/lib/libcpu/i386/cpu.h            |  83 ++++++++++++++++++-
>>   2 files changed, 194 insertions(+), 36 deletions(-)
>>
>> diff --git a/c/src/lib/libbsp/i386/shared/irq/idt.c b/c/src/lib/libbsp/i386/shared/irq/idt.c
>> index b79c60a..e6f1d57 100644
>> --- a/c/src/lib/libbsp/i386/shared/irq/idt.c
>> +++ b/c/src/lib/libbsp/i386/shared/irq/idt.c
>> @@ -229,50 +229,33 @@ int i386_get_idt_config (rtems_raw_irq_global_settings** config)
>>     return 1;
>>   }
>>
>> -/*
>> - * Caution this function assumes the GDTR has been already set.
>> - */
>> -int i386_set_gdt_entry (unsigned short segment_selector, unsigned base,
>> -                       unsigned limit)
>> +int i386_raw_gdt_entry (unsigned short segment_selector_index, segment_descriptors* sd)
>
> Break apart lines > 80 characters, or shorten the line (by shortening
> variable names).
ok

>
>>   {
>> -    unsigned                   gdt_limit;
>> +    unsigned                    gdt_limit;
>>       unsigned short              tmp_segment = 0;
>> -    unsigned int                limit_adjusted;
>> -    segment_descriptors*       gdt_entry_tbl;
>> +    segment_descriptors*        gdt_entry_tbl;
>> +    unsigned int present;
>>
>>       i386_get_info_from_GDTR (&gdt_entry_tbl, &gdt_limit);
>>
>> -    if (segment_selector > limit) {
>> +    if (segment_selector_index >= (gdt_limit+1)/8) {
>> +      /* index to GDT table out of bounds */
>>         return 0;
>>       }
>> -    /*
>> -     * set up limit first
>> -     */
>> -    limit_adjusted = limit;
>> -    if ( limit > 4095 ) {
>> -      gdt_entry_tbl[segment_selector].granularity = 1;
>> -      limit_adjusted /= 4096;
>> +    if (segment_selector_index == 0) {
>> +      /* index 0 is not usable */
>> +      return 0;
>>       }
>> -    gdt_entry_tbl[segment_selector].limit_15_0  = limit_adjusted & 0xffff;
>> -    gdt_entry_tbl[segment_selector].limit_19_16 = (limit_adjusted >> 16) & 0xf;
>> -    /*
>> -     * set up base
>> -     */
>> -    gdt_entry_tbl[segment_selector].base_address_15_0  = base & 0xffff;
>> -    gdt_entry_tbl[segment_selector].base_address_23_16 = (base >> 16) & 0xff;
>> -    gdt_entry_tbl[segment_selector].base_address_31_24 = (base >> 24) & 0xff;
>> -    /*
>> -     * set up descriptor type (this may well becomes a parameter if needed)
>> -     */
>> -    gdt_entry_tbl[segment_selector].type               = 2;    /* Data R/W */
>> -    gdt_entry_tbl[segment_selector].descriptor_type    = 1;    /* Code or Data */
>> -    gdt_entry_tbl[segment_selector].privilege          = 0;    /* ring 0 */
>> -    gdt_entry_tbl[segment_selector].present            = 1;    /* not present */
>>
>> +    /* put prepared descriptor into the GDT */
>> +    present = sd->present;
>> +    sd->present = 0;
>> +    gdt_entry_tbl[segment_selector_index] = *sd;
>> +    sd->present = present;
>> +    gdt_entry_tbl[segment_selector_index].present = present;
> Why write the present field separately from the rest?
It was meant as a protection from using partly written descriptor. While
present bit is set to 0 and an instruction tries to load it, processor
generates exception.

Better code for this to work correctly would be:
     /* put prepared descriptor into the GDT */
     present = sd->present;
     sd->present = 0;
     gdt_entry_tbl[segment_selector_index].present = 0;
     RTEMS_COMPILER_MEMORY_BARRIER();
     gdt_entry_tbl[segment_selector_index] = *sd;
     RTEMS_COMPILER_MEMORY_BARRIER();
     gdt_entry_tbl[segment_selector_index].present = present;
     sd->present = present;

>
>>       /*
>> -     * Now, reload all segment registers so the limit takes effect.
>> +     * Now, reload all segment registers so that the possible changes takes effect.
>>        */
>> -
>>       __asm__ volatile( "movw %%ds,%0 ; movw %0,%%ds\n\t"
>>                     "movw %%es,%0 ; movw %0,%%es\n\t"
>>                     "movw %%fs,%0 ; movw %0,%%fs\n\t"
>> @@ -280,7 +263,101 @@ int i386_set_gdt_entry (unsigned short segment_selector, unsigned base,
>>                     "movw %%ss,%0 ; movw %0,%%ss"
>>                      : "=r" (tmp_segment)
>>                      : "0"  (tmp_segment)
>> -                 );
>> -
>> +                 );
>>       return 1;
>>   }
>> +
>> +inline void i386_fill_segment_desc_base(unsigned base, segment_descriptors* sd)
>> +{
>> +    sd->base_address_15_0  = base & 0xffff;
>> +    sd->base_address_23_16 = (base >> 16) & 0xff;
>> +    sd->base_address_31_24 = (base >> 24) & 0xff;
>> +}
>> +
>> +inline void i386_fill_segment_desc_limit(unsigned limit, segment_descriptors* sd)
>
> Anytime you use inline, you should use static. If the function should
> be available to other files, it should go in a header. The same goes
> for any non-static function, they should have prototypes declared in a
> header.
ok, I made from these non-inline functions.

>
>> +{
>> +    sd->granularity = 0;
>> +    if (limit > 65535) {
>> +      sd->granularity = 1;
>> +      limit /= 4096;
>> +    }
>> +    sd->limit_15_0  = limit & 0xffff;
>> +    sd->limit_19_16 = (limit >> 16) & 0xf;
>> +}
>> +
>> +/*
>> + * Caution this function assumes the GDTR has been already set.
>> + */
>> +int i386_set_gdt_entry (unsigned short segment_selector_index, unsigned base,
>> +                        unsigned limit)
>> +{
>> +    segment_descriptors         gdt_entry;
>> +    memset(&gdt_entry, 0, sizeof(gdt_entry));
>> +
>> +    i386_fill_segment_desc_limit(limit, &gdt_entry);
>> +    i386_fill_segment_desc_base(base, &gdt_entry);
>> +    /*
>> +     * set up descriptor type (this may well becomes a parameter if needed)
>> +     */
>> +    gdt_entry.type              = 2;    /* Data R/W */
>> +    gdt_entry.descriptor_type   = 1;    /* Code or Data */
>> +    gdt_entry.privilege         = 0;    /* ring 0 */
>> +    gdt_entry.present           = 1;    /* not present */
>> +
>> +    /*
>> +     * Now, reload all segment registers so the limit takes effect.
>> +     */
>> +    return i386_raw_gdt_entry(segment_selector_index, &gdt_entry);
>> +}
>> +
>> +unsigned short i386_next_empty_gdt_entry ()
>> +{
>> +    unsigned                    gdt_limit;
>> +    segment_descriptors*        gdt_entry_tbl;
>> +    /* initial amount of filled descriptors */
>> +    static unsigned short       segment_selector_index = 2;
>> +
>> +    segment_selector_index += 1;
>> +    i386_get_info_from_GDTR (&gdt_entry_tbl, &gdt_limit);
>> +    if (segment_selector_index >= (gdt_limit+1)/8) {
>> +      return 0;
>> +    }
>> +    return segment_selector_index;
>> +}
>> +
>> +unsigned short i386_cpy_gdt_entry(unsigned short segment_selector_index, segment_descriptors* strucToFill)
>
> Prefer to avoid camelNotation. Also check for lines longer than 80 characters.
ok

>
> Is there a reason that sometimes you are using unsigned, and sometimes
> unsigned short? I don't like seeing the mixing of int types, and I
> would prefer to see explicit-width (e.g. uint32_t and uint16_t) types
> if the width is important.
It is not completely true for this patch, but in my working branch it's 
done:
The reason is to use the same type of variable as the function's
parameter type the variable goes into. e.g.
i386_get_info_from_GDTR have second parameter of type 'unsigned' so
gdt_limit which goes in there has is also 'unsigned' and
'segment_selector_index' which is compared with gdt_limit is also
unsigned (not in this patch).

I could use explicit-width type as you suggested and then cast it to
(unsigned) or change parameters types of function involved.

'unsigned int' is used for bit fields in segment_descriptors structure,
so I stick with this type when manipulating structure mentioned.

Any ideas are very welcomed.
>
>> +{
>> +    unsigned                    gdt_limit;
>> +    segment_descriptors*        gdt_entry_tbl;
>> +
>> +    i386_get_info_from_GDTR (&gdt_entry_tbl, &gdt_limit);
>> +
>> +    if (segment_selector_index >= (gdt_limit+1)/8) {
>> +      return 0;
>> +    }
>> +
>> +    *strucToFill = gdt_entry_tbl[segment_selector_index];
>> +    return segment_selector_index;
>> +}
>> +
>> +segment_descriptors* i386_get_gdt_entry(unsigned short segment_selector_index)
>> +{
>> +    unsigned                    gdt_limit;
>> +    segment_descriptors*        gdt_entry_tbl;
>> +
>> +    i386_get_info_from_GDTR (&gdt_entry_tbl, &gdt_limit);
>> +
>> +    if (segment_selector_index >= (gdt_limit+1)/8) {
>> +      return 0;
>> +    }
>> +    return &gdt_entry_tbl[segment_selector_index];
>> +}
>> +
>> +unsigned i386_limit_gdt_entry(segment_descriptors* gdt_entry)
>> +{
>> +    unsigned lim = (gdt_entry->limit_15_0 + (gdt_entry->limit_19_16<<16));
>> +    if (gdt_entry->granularity) {
>> +      return lim*4096+4095;
>> +    }
>> +    return lim;
>> +}
>> diff --git a/c/src/lib/libcpu/i386/cpu.h b/c/src/lib/libcpu/i386/cpu.h
>> index 4c56731..049a35c 100644
>> --- a/c/src/lib/libcpu/i386/cpu.h
>> +++ b/c/src/lib/libcpu/i386/cpu.h
>> @@ -269,11 +269,92 @@ extern void i386_get_info_from_GDTR (segment_descriptors** table,
>>   extern void i386_set_GDTR (segment_descriptors*,
>>                             unsigned limit);
>>
>> +/**
>> + * C callable function:
>> + * Puts global descriptor @sd to the global descriptor table on index
>> + * @segment_selector_index
>> + *
>> + * @return  0 FAILED out of GDT range or index is 0, which is not valid
>> + *                   index in GDT
>> + *          1 SUCCESS
>> + */
>> +extern int i386_raw_gdt_entry ( unsigned short segment_selector_index,
>> +                                segment_descriptors* sd);
>> +
>> +/**
>> + * C callable function
>> + * fills @sd with provided @base in appropriate fields of @sd
>> + *
>> + * @param base 32-bit address to be set as descriptor's base
>> + * @param sd descriptor being filled with @base
>> + */
>> +extern void i386_fill_segment_desc_base(unsigned base, segment_descriptors* sd);
>> +
>> +/**
>> + * C callable function
>> + * fills @sd with provided @limit in appropriate fields of @sd
>> + * also influences granularity bit
>> + *
>> + * @param limit 32-bit value representing number of limit bytes
>> + * @param sd descriptor being filled with @limit
>> + */
>> +extern void i386_fill_segment_desc_limit(unsigned limit,
>> +                                        segment_descriptors* sd);
>> +
>>   /*
>>    * C callable function enabling to set up one raw interrupt handler
>>    */
>>   extern int i386_set_gdt_entry (unsigned short segment_selector, unsigned base,
>> -                                            unsigned limit);
>> +                                unsigned limit);
>> +
>> +/**
>> + * C callable function returns next empty descriptor in GDT.
>> + *
>> + * @return  0 FAILED GDT is full
>> + *          <1;65535> segment_selector number as index to GDT
>> + */
>> +extern unsigned short i386_next_empty_gdt_entry (void);
>> +
>> +/**
>> + * Copies GDT entry at index @segment_selector to structure
>> + * pointed to by @strucToFill
>> + *
>> + * @param  segment_selector index to GDT table for specifying descriptor to copy
>> + * @return  0 FAILED segment_selector out of GDT range
>> + *          <1;65535> retrieved segment_selector
>> + */
>> +extern unsigned short i386_cpy_gdt_entry(unsigned short segment_selector,
>> +                                        segment_descriptors* strucToFill);
>> +
>> +/**
>> + * Returns pointer to GDT table at index given by @segment_selector
>> + *
>> + * @param   segment_selector index to GDT table for specifying descriptor to get
>> + * @return  NULL FAILED segment_selector out of GDT range
>> + *          pointer to GDT table at @segment_selector
>> + */
>> +extern segment_descriptors* i386_get_gdt_entry(unsigned short segment_selector);
>> +
>> +/**
>> + * Extracts base address from GDT entry pointed to by @gdt_entry
>> + *
>> + * @param  gdt_entry pointer to entry from which base should be retrieved
>> + * @return base address from GDT entry
>> +*/
>> +extern inline void* i386_base_gdt_entry(segment_descriptors* gdt_entry)
>> +{
>> +    return (void*)(gdt_entry->base_address_15_0 +
>> +            (gdt_entry->base_address_23_16<<16) +
>> +            (gdt_entry->base_address_31_24<<24));
>> +}
> use static inline. There is a macro RTEMS_INLINE_ROUTINE that probably
> should be used. also, using bitwise-or instead of addition seems
> better for composing this field, although I don't have any
> particularly compelling reason.
ok

>
>> +
>> +/**
>> + * Extracts limit in bytes from GDT entry pointed to by @gdt_entry
>> + *
>> + * @param  gdt_entry pointer to entry from which limit should be retrieved
>> + * @return limit value in bytes from GDT entry
>> + */
>> +extern unsigned i386_limit_gdt_entry(segment_descriptors* gdt_entry);
>>
>>   /*
>>    * See page 11.18 Figure 11-12.
>> --
>> 1.9.1
>>
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel



More information about the devel mailing list