Re: [PATCH 1/1] pickles: Add new PCI Pickle

[Mohammad-Reza Nabipoor]

Hi Darshit.

Thank you for the patch.


On Tue, Sep 17, 2024 at 08:33:42PM GMT, Darshit Shah wrote:
> diff --git a/pickles/pci/pci_common.pk b/pickles/pci/pci_common.pk
> new file mode 100644
> index 0000000..4e91745
> --- /dev/null
> +++ b/pickles/pci/pci_common.pk
[...]
> +/**
> + * Basic Registers
> + */
> +type reg_32 = uint<32>;
> +type reg_16 = uint<16>;
> +type reg_8 = uint<8>;


Do we really need these types and uint<...> is not readable enough?
If yes, please use names like `PCI_Reg32`, `PCI_Reg16` and `PCI_Reg8`.


> +
> +//  While the PCI base specification differentiates between RsvdZ and RsvdP, 
> we
> +//  don't need that in Poke. The only difference between the two is what 
> happens
> +//  when you write to the bit, which is not something our representation 
> needs
> +//  to care about.
> +type rsvdz = struct bit {
> +    bit rsvdz == 0x0;
> +    method _print = void:
> +    {
> +        print "rsvdz: 0";
> +    }
> +};


We use the following code style:

type PIC_RsvdZ =
  struct
    {
      uint<1> rsvdz == 0;

      // ...
    };

And `bit' is defined in `std-types.pk' file.
This file is not loaded by some users of `libpoke' (like `gdb' integration).
To make this pickle also useful for those applications, it's better to not use
these "Standard Types".

> -- 
> 2.46.1
> 
> 
> 
> diff --git a/pickles/pci/pci.pk b/pickles/pci/pci.pk
> new file mode 100644
> index 0000000..10fb5ac
> --- /dev/null
> +++ b/pickles/pci/pci.pk
> +
> +type PCI_Command = struct reg_16 {
> +    rsvdz;
> +    rsvdz;
> +    rsvdz;
> +    rsvdz;
> +    rsvdz;
> +    bit int_disable;
> +    bit fast_b2b_tx_enable == 0b0;


Instead of `0b0' you can use `0' or `0T' or `0t`.
`0T' and `0t' are of type `uint<1>'.
`0' and `0b0' are of type `int<32>' (which is compatible due to implicit
conversions).


> +    bit serr_enable;
> +    bit idsel_stepping == 0b0;
> +    bit parity_error_response;
> +    bit vga_palette_snoop == 0b0;
> +    bit mem_write_invalidate == 0b0;
> +    bit sp_cycle_enable == 0b0;
> +    bit bus_master_enable;
> +    bit mem_enable;
> +    bit io_enable;
> +};
> +
> +type PCI_Status = struct reg_16 {
> +    bit parity_error;
> +    bit sig_sys_error;
> +    bit rcvd_master_abort;
> +    bit rcvd_target_abort;
> +    bit sig_target_abort;
> +    uint<2> devsel_timing == 0 as uint<2>;

The `as uint<2>' is not necessary.

> +    bit master_data_parity_error;
> +    bit fast_b2b_tx_cap;
> +    rsvdz;
> +    bit Mhz66_cap == 0b0;
> +    bit cap_list;
> +    bit int_status;
> +    rsvdz;
> +    rsvdz;
> +    bit imm_readiness;
> +};
> +
> +type PCI_Hdr_Type = struct reg_8 {
> +    uint<7> hdr_layout;
> +    bit multi_fn;
> +};
> +
> +type PCI_BAR = struct {
> +    reg_32;
> +};
> +
> +type PCI_Type0 = struct {
> +    PCI_BAR[6] bars;
> +    reg_32 cardbus_cis_ptr == 0x0U;
> +    reg_16 subsystem_vendor_id;
> +    reg_16 subsystem_id;
> +    reg_32 exp_rom_base_addr;
> +    offset<reg_8,B> cap_ptr;
> +    uint<56> rsvdz == 0;
> +    reg_8 int_line;
> +    reg_8 int_pin;
> +    reg_8 min_gnt;
> +    reg_8 max_lat;
> +};


It is a good practice to add a size assertion after defining these
types like: `assert (#PCI_Type0 == 48#B);'
```


> +
> +type PCI_Type1  = struct {
> +    PCI_BAR bar_0;
> +    PCI_BAR bar_1;
> +    reg_8 primary_bus_nr;
> +    reg_8 sec_bus_nr;
> +    reg_8 sub_bus_nr;
> +    reg_8 sec_lat_timer;
> +    reg_8 io_base;
> +    reg_8 io_limit;
> +    reg_16 sec_status;
> +    reg_16 mem_base;
> +    reg_16 mem_limit;
> +    reg_16 prefetch_mem_base;
> +    reg_16 prefetch_mem_limit;
> +    reg_32 prefetch_mem_base_upper;
> +    reg_32 prefetch_mem_limit_upper;
> +    reg_16 io_base_upper;
> +    reg_16 io_base_limit;
> +    offset<reg_16,B> cap_ptr;
> +    uint<24> rsvdz == 0;
> +    reg_32 exp_rom_base;
> +    reg_8 int_line;
> +    reg_8 int_pin;
> +    reg_16 bridge_ctrl;
> +};
> +
> +
> +type PCI_Config_Space = struct {
> +    reg_16 vendor;
> +    reg_16 device;
> +    PCI_Command command;
> +    PCI_Status status;
> +    reg_8 revision_id;
> +    uint<24> class_code;
> +    reg_8 cache_line_size;
> +    reg_8 pri_lat_timer == 0;
> +    PCI_Hdr_Type header;
> +    reg_8 BIST;
> +    if (header.hdr_layout == 0b0)
> +        PCI_Type0 endpt_specific_data;
> +    if (header.hdr_layout == 0b1)
> +        PCI_Type1 bridge_specific_data;
> +    // These are technically not a part of Type 0 / Type 1 structures
> +    // But I define them in there for ease. Instead, we can use Field Labels
> +    // to create virtual fields here that point to the correct data
> +    reg_8 cap_ptr @ 0x34#B;
> +    reg_8 int_line @ 0x3C#B;
> +    reg_8 int_pin @ 0x3D#B;


Instead of field labels (which only works for mapped data), you can use 
`computed' fields:

```
      computed uint<8> cap_ptr;
      computed uint<8> int_line;
      computed uint<8> int_pin;

      method get_cap_ptr = uint<8>:
        { return header.hdr_layout == 0 ? endpt_specific_data.cap_ptr : /* ... 
*/ ; }
      method get_int_line = uint<8>: {/*...*/}
      method get_int_pin  = uint<8>: {/*...*/}

      method set_cap_ptr = (uint<8> cap_ptr) void: { /* ... */ }
      // ...
```


> +};
> +
> +assert ((PCI_Config_Space {})'size == 0x40#B);



Regards,
Mohammad-Reza