On Tue, Nov 18, 2014 at 10:46 AM, Jan Dolezal <dolez...@fel.cvut.cz> wrote:
>
> On 12.11.2014 16:42, Gedare Bloom wrote:
>>
>> On Wed, Nov 12, 2014 at 10:07 AM, Jan Dolezal <dolez...@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@rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
