sdbfis not defined in AHCI wiki article?

[Ethin]

Not sure if this was an accidental or deliberate oversight, but the AHCI wiki article references an undefined 'FIS_DEV_BITS' identifier. Looking at the Haiku source code for this FIS, its an array of size 8 of type uint8_t. The SATA standard (v. 3.4) defines this FIS as:

Error(7:0) R Status Hi R Status Lo N I R R PM Port FIS Type (A1h)
Protocol Specific

Or at least, that's what it looks like to me in my PDF reader. If the structure is the same as the other FISes, would it looke like this?

Code: Select all

typedef struct tagFIS_SET_DEVICE_BITS {
  uint8_t fis_type;
  uint8_t pmport:4;
  uint8_t rsvd[2];
  uint8_t i:1;
  uint8_t n:1;
  uint8_t statusl;
  uint8_t rsvd2:1;
  uint8_t statush;
  uint8_t rsvd3:1;
  uint8_t error;
} FIS_DEV_BITS;

The figure is figure 277 in SATA 3.4,section 10.5.7.1.

[Octocontrabass]

I think you've got some typos in there. It should add up to exactly four bytes. (Or eight bytes, if you include the protocol-specific part.)

Try this:

Code: Select all

typedef struct tagFIS_SET_DEVICE_BITS {
  uint8_t fis_type;
  uint8_t pmport:4;
  uint8_t rsvd:2;
  uint8_t i:1;
  uint8_t n:1;
  uint8_t statusl:3;
  uint8_t rsvd2:1;
  uint8_t statush:3;
  uint8_t rsvd3:1;
  uint8_t error;
} FIS_DEV_BITS;

[Ethin]

I think you've got some typos in there. It should add up to exactly four bytes. (Or eight bytes, if you include the protocol-specific part.)

Try this:

Code: Select all

typedef struct tagFIS_SET_DEVICE_BITS {
  uint8_t fis_type;
  uint8_t pmport:4;
  uint8_t rsvd:2;
  uint8_t i:1;
  uint8_t n:1;
  uint8_t statusl:3;
  uint8_t rsvd2:1;
  uint8_t statush:3;
  uint8_t rsvd3:1;
  uint8_t error;
} FIS_DEV_BITS;

Problem is that I'm writing this in Rust (which doesn't have bitfields). So, that would be about 10 bytes or so.

[Octocontrabass]

The C bit fields are just a convenient way to represent how the data is packed. If your language of choice doesn't have bit fields, then you can use an array of integers and manually extract the bit fields using shifts and masks.

[nullplan]

I object to the words "bitfield" and "convenient" being placed in the same sentence without a negation in between. Especially when it is for a structure you are supposed to transmit somewhere. I've had so many endianess problems with them, it is not even funny anymore. Just use shifts and masks, that way at least you know where your bits are going.

[Ethin]

So define the struct as you guys indicated earlier, than use shifts and masks to extract (and set) those bits? Just confirming. So I might define the structure the same way but with all u8s?

[thewrongchristian]

I object to the words "bitfield" and "convenient" being placed in the same sentence without a negation in between. Especially when it is for a structure you are supposed to transmit somewhere. I've had so many endianess problems with them, it is not even funny anymore. Just use shifts and masks, that way at least you know where your bits are going.

+1 Like

The ordering and packing of bitfields isn't even defined as far as I know.

[Octocontrabass]

So define the struct as you guys indicated earlier, than use shifts and masks to extract (and set) those bits? Just confirming. So I might define the structure the same way but with all u8s?

If you're not going to be using it to interact with the hardware, then you can define the structure however you want. For example, you might leave out the reserved bits since you won't be able to interpret them anyway.

The important part is using packed bits when you interact with the hardware. How you pack and unpack the bits is up to you. (As others have noted, C bitfields are not the most portable way of doing it, since they are ABI-dependent. I still think they're a good way to visualize the structure though.)

[Ethin]

So define the struct as you guys indicated earlier, than use shifts and masks to extract (and set) those bits? Just confirming. So I might define the structure the same way but with all u8s?

If you're not going to be using it to interact with the hardware, then you can define the structure however you want. For example, you might leave out the reserved bits since you won't be able to interpret them anyway.

The important part is using packed bits when you interact with the hardware. How you pack and unpack the bits is up to you. (As others have noted, C bitfields are not the most portable way of doing it, since they are ABI-dependent. I still think they're a good way to visualize the structure though.)

I do want to use them to interact with the hardware, so I've defined them as all u8s (since I can't use bitfields, and I presume that if I use a complex structure like a bit vector the structure will read in and write out data that's beyond the bounds of the actual structure in HW memory). Bitfields are nice, though I'm kinda confused.... aren't they just a way of limiting the range of an integer? Like if you define a u8 that takes up 5 bits, its still going to take up the remaining bits, you just won't be able to access them?
Edit: Oh, I see now... the C standard says: "An implementation may allocate any addressable storage unit large enough to hold a bit-field. If enough space remains, a bit-field that immediately follows another bit-field in a structure shall be packed into adjacent bits of the same unit. If insufficient space remains, whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is implementation-defined. The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined. The alignment of the addressable storage unit is unspecified." I'm not really sure how I could accomplish such a thing in Rust, which is what I'm using... It doesn't support bitfields at the moment, and even if I use the packed representation I don't know how accurate that comes to bitfields. The Rustonomicon has this to say on the packed representation: "repr(packed) forces Rust to strip any padding, and only align the type to a byte. This may improve the memory footprint, but will likely have other negative side-effects.
In particular, most architectures strongly prefer values to be aligned. This may mean the unaligned loads are penalized (x86), or even fault (some ARM chips). For simple cases like directly loading or storing a packed field, the compiler might be able to paper over alignment issues with shifts and masks. However if you take a reference to a packed field, it's unlikely that the compiler will be able to emit code to avoid an unaligned load.
As of Rust 2018, this still can cause undefined behavior.
repr(packed) is not to be used lightly. Unless you have extreme requirements, this should not be used.
This repr is a modifier on repr(C) and repr(rust)."

[Ethin]

Update: so I'm not sure if I'll do AHCI right now, purely because of the difficulty with bitfields in rust at the moment. I was thinking that my kernel would have a minimal ATA PIO driver built-in, enough to be able to read sectors and write, and then I could later work on a more expansive AHCI crate once I got this mess sorted out. (I could have bindgen generate rust equivalents to the C code on the wiki, but the problem with that is that it will generate about 600 lines of unnecessary code just to "emulate" the bitfields because it follows LLVM IR.) Not to mention I still have more AHCI-based questions and problems I need to overcome (like finding good, idiomatic Rust ways of doing FISes in Rust). I've started a pretty interesting discussion over here on the Rust user forums for the HBA port array. A similar problem exists for the PRDT entries array; I can't create an array of 65536 elements on the stack, not only because that's utterly overkill, but because it will (1) waste huge amounts of stack space and cause problems in the future and (2) exponentially increase compilation times.

[thewrongchristian]

Update: so I'm not sure if I'll do AHCI right now, purely because of the difficulty with bitfields in rust at the moment. I was thinking that my kernel would have a minimal ATA PIO driver built-in, enough to be able to read sectors and write, and then I could later work on a more expansive AHCI crate once I got this mess sorted out. (I could have bindgen generate rust equivalents to the C code on the wiki, but the problem with that is that it will generate about 600 lines of unnecessary code just to "emulate" the bitfields because it follows LLVM IR.) Not to mention I still have more AHCI-based questions and problems I need to overcome (like finding good, idiomatic Rust ways of doing FISes in Rust). I've started a pretty interesting discussion over here on the Rust user forums for the HBA port array. A similar problem exists for the PRDT entries array; I can't create an array of 65536 elements on the stack, not only because that's utterly overkill, but because it will (1) waste huge amounts of stack space and cause problems in the future and (2) exponentially increase compilation times.

I have zero clue in rust, but doesn't it provide bit field manipulation in a library?

https://docs.rs/bit_field/0.10.0/bit_field/index.html

[Ethin]

Update: so I'm not sure if I'll do AHCI right now, purely because of the difficulty with bitfields in rust at the moment. I was thinking that my kernel would have a minimal ATA PIO driver built-in, enough to be able to read sectors and write, and then I could later work on a more expansive AHCI crate once I got this mess sorted out. (I could have bindgen generate rust equivalents to the C code on the wiki, but the problem with that is that it will generate about 600 lines of unnecessary code just to "emulate" the bitfields because it follows LLVM IR.) Not to mention I still have more AHCI-based questions and problems I need to overcome (like finding good, idiomatic Rust ways of doing FISes in Rust). I've started a pretty interesting discussion over here on the Rust user forums for the HBA port array. A similar problem exists for the PRDT entries array; I can't create an array of 65536 elements on the stack, not only because that's utterly overkill, but because it will (1) waste huge amounts of stack space and cause problems in the future and (2) exponentially increase compilation times.

I have zero clue in rust, but doesn't it provide bit field manipulation in a library?

https://docs.rs/bit_field/0.10.0/bit_field/index.html

Yep, it does, and I use that extensively throughout my kernel. But its not bitfields in the style of C bitfields. It defines implementations of traits (think classes that implement interfaces) on the various primitive integer types for bit modification. I could do things that way, but I would then need to heavily modify the AHCI structures to account for that change. I could have "i" be its own field then, for example -- it would need to be packed into the previous field, I think.

[Octocontrabass]

Perhaps what you want is an array of bytes along with getters and setters that use the bit field crate (or plain shifts and masks) to access the different fields.

I'm not familiar with Rust, but maybe it has some syntax to make that kind of thing almost as pretty as a struct?