Re: [Qemu-devel] Ping [PATCH 0/2] Add TPCI200 and IP-Octal 232 IndustryPack emulation

[Avi Kivity <avi@redhat.com>]

On 10/10/2012 12:24 PM, Alberto Garcia wrote:
> On Sun, Oct 07, 2012 at 12:13:53PM +0200, Avi Kivity wrote:
> 
>> Luckily the low-order bits are used for offsets, and the high-order
>> bits are used for selecting the sub-device.
>> 
>> So you could easily have
>> 
>>  struct IPackDevice {
>>      DeviceState qdev;
>>      int32_t slot;
>>      /* IRQ objects for the IndustryPack INT0# and INT1# */
>>      qemu_irq *irq;
>>      MemoryRegion io_space;
>>      MemoryRegion id_space;
>>      MemoryRegion int_space;
>>      MemoryRegion mem8_space;  /* for las3 */
>>      MemoryRegion mem16_space; /* for las2 */
>>  };
>> 
>> The PCI device would then just map each space (with
>> memory_region_add_subregion()) into las1/las2/las3 such that the
>> high bits select the device/space.  The low bits would automatically
>> become the offset into the space.
> 
> Hey, I finally found some time to look into this, the problem that I
> see is that the PCI carrier doesn't just map each space into its local
> address spaces, in addition to that:
> 
>   1) it changes the data and addresses according to the endianness
>      configuration in the local configuration registers (PCI BAR0).
>      See adjust_addr()/adjust_value() and page 20 of the user manual.
> 
>      http://www.tews.com/Products/ArticleGroup/TPCI/TPCI200.html

This is supported (albeit not cleanly) by the memory API.  There is the
MemoryRegionOps::endianess attribute, you can have a mem16_be_space and
mem16_le_space; use
memory_region_add_subregion()/memory_region_del_subregion() to map
le_space or be_space as needed to the BAR.

> 
>   2) read accesses to the local address space 1 are also used to
>      acknowledge interrupts (manual page 33 and tpci200_read_las1()).
> 

You can map a small subregion over those addresses.  What is the value
read?  does it need to go to the int space and read it, or is it some
fixed value?

> 
> 
> On a related note, I was wondering whether it would be simpler to get
> rid of tpci200_cfg_ops and replace the current implementation of the
> local configuration registers with something like this:
> 
> memory_region_init_ram_ptr(&s->mmio, "tpci200_mmio", 128, s->local_cfg);
> memory_region_init_alias(&s->io, "tpci200_io", &s->mmio, 0, 128);
> pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mmio);
> pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_IO,     &s->io);
> 
> (s->local_cfg would contain the config registers data)
> 
> But it doesn't seem to work.

This is supposed to work.  What does 'info mtree' show?  What happens if
you grow the size up to a page size?  Do both BARs fail, or just the IO BAR?


-- 
error compiling committee.c: too many arguments to function