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

Gedare Bloom gedare at rtems.org
Tue Nov 18 16:10:06 UTC 2014


On Tue, Nov 18, 2014 at 10:46 AM, Jan Dolezal <dolezj21 at fel.cvut.cz> wrote:
>
> 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.
>
I would prefer to see the explicit width types, and to change the
existing functions to use explicit width also. You should propose the
change to existing functions as a separate patch that should be
applied before yours.

-Gedare

>>
>>> +{
>>> +    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