Wrong definition for the "x86_x32_tss_t" class ?

[Wilfried Catteau]

Hi,

in the file "/kernel/src/arch/x86/x32/tss.h".

The definition of the "io_bitmap" member seems to be wrong for me:

u8_t io_bitmap[IOPERMBITMAP_SIZE+1] X86_X32_IOPERMBITMAP_ALIGNMENT;

I suppose the next member named "stopper" is necessary to avoid the
processor to generate an exception when dealing with the last byte of
the io bitmap array, like said in the Intel documentation. In that case,
adding one byte is not necessary and lead to a problem: the
"init_io_space()" function only fill 'IOPERMBITMAP_SIZE' bytes. So,
there is a byte that has an undefined contents between the "io_bitmap"
array and the "stopper" byte.

Why do you use an alignment restriction ? I didn't see it in the Intel
documentation.

Willy.

[Raphael Neider]

Hi Willy,

> u8_t io_bitmap[IOPERMBITMAP_SIZE+1] X86_X32_IOPERMBITMAP_ALIGNMENT;
>
> I suppose the next member named "stopper" is necessary to avoid the  
> processor to generate an exception when dealing with the last byte of the
> io bitmap array, like said in the Intel documentation. In that case,

I guess so, too.

> adding one byte is not necessary and lead to a problem: the  
> "init_io_space()" function only fill 'IOPERMBITMAP_SIZE' bytes. So, there
> is a byte that has an undefined contents between the "io_bitmap" array and
> the "stopper" byte.

You are probably right. Luckily this will only be a problem if one of
the I/O ports 65528..65535 (0xFFF8..0xFFFF) is accessed. Probably this
never happens (I am not aware of any device using these ports).
This should eventually be fixed (or documented) both in the x32 and the
x64 variants. Thank you for the report.

> Why do you use an alignment restriction ? I didn't see it in the Intel  
> documentation.

Looking at the definition of X86_X32_IOPERMBITMAP_ALIGNMENT in lines 36--42
I conclude that the alignment has to do with I/O flexpages. Possibly
our implementation of mapping I/O permissions simply require(d|s) that the
bitmap is page aligned. I did not check this, though ...

Regards,
Raphael

[Mark P. Jones]

Raphael Neider wrote:
>> Why do you use an alignment restriction ? I didn't see it in the Intel  
>> documentation.
>
> Looking at the definition of X86_X32_IOPERMBITMAP_ALIGNMENT in lines 36--42
> I conclude that the alignment has to do with I/O flexpages. Possibly
> our implementation of mapping I/O permissions simply require(d|s) that the
> bitmap is page aligned. I did not check this, though ...

I believe you'll find an explanation for the alignment restriction in
the following report (see the top of Page 16):

http://i30www.ira.uka.de/teaching/thesisdocuments/l4ka/2002/stoess_st_io-flexpages.pdf

Hope that helps!

All the best,
Mark

[Jan Stoess]

> Looking at the definition of X86_X32_IOPERMBITMAP_ALIGNMENT in lines
> 36--42
> I conclude that the alignment has to do with I/O flexpages. Possibly
> our implementation of mapping I/O permissions simply require(d|s) that
> the
> bitmap is page aligned. I did not check this, though ...

The reason is that we transparently switch I/O bitmaps on address space switches; that requires the two bitmap pages to be aligned on a 4K boundary.

-Jan

--
Jan Stoess
System Architecture Group
University of Karlsruhe
Phone: [phone]
Fax: [phone]
eMail: stoess@...

[Jan Stoess]

> I suppose the next member named "stopper" is necessary to avoid the
> processor to generate an exception when dealing with the last byte of
> the io bitmap array, like said in the Intel documentation. In that
> case,
> adding one byte is not necessary and lead to a problem: the
> "init_io_space()" function only fill 'IOPERMBITMAP_SIZE' bytes. So,
> there is a byte that has an undefined contents between the "io_bitmap"
> array and the "stopper" byte.


You're right, stopper and IOPERMBITMAPS_SIZE is redundant. I've fixed it in the tree.

> > 2 - In the file "/kernel/src/arch/x86/x32/segdesc.h" : In lines 80
> and
> > 102, it is more logical to use 0xf in place of 0xff as the limit_high
> > field is only 4 bits.
>
> > 3 - In the file "/kernel/src/arch/x86/segdesc.h" : In the
> > "x86_descreg_t", why do you use the member
> > "pad[sizeof(word_t)-sizeof(u16_t)]" in the union ? The GDTR and IDTR
> > registers are 48 bits (address + limit) and the TR and LDTR registers
> are
> > only 16 bits (selector) even if they are internally larger (the
> hidden
> > part, only accessible by processor).
>

Fixed them in the tree as well.

-Jan
--
Jan Stoess
System Architecture Group
University of Karlsruhe
Phone: [phone]
Fax: [phone]
eMail: stoess@...>