Re: [Qemu-devel] Re: [RFC PATCH] AMD IOMMU emulation

2010-05-25 Thread Blue Swirl
On Tue, May 25, 2010 at 8:39 AM, Joerg Roedel  wrote:
> On Mon, May 24, 2010 at 08:10:16PM +, Blue Swirl wrote:
>> On Mon, May 24, 2010 at 3:40 PM, Joerg Roedel  wrote:
>> >> +
>> >> +#define MMIO_SIZE               0x2028
>> >
>> > This size should be a power-of-two value. In this case probably 0x4000.
>>
>> Not really, the devices can reserve regions of any size. There were
>> some implementation deficiencies in earlier versions of QEMU, where
>> the whole page would be reserved anyway, but this limitation has been
>> removed long time ago.
>
> The drivers for AMD IOMMU expect that to be 0x4000. At least the Linux
> driver maps the MMIO region with this size. So the emulation should
> reserve this amount of MMIO space too.

Well, Linux drivers may take a conservative approach so I'd check
what's the value in the device specs. In practice, on x86 hardware the
size doesn't matter too much, for example on Sparc an access beyond
the end of the device region would trap.



Re: [Qemu-devel] Re: [RFC PATCH] AMD IOMMU emulation

2010-05-25 Thread Eduard - Gabriel Munteanu
On Tue, May 25, 2010 at 10:39:22AM +0200, Joerg Roedel wrote:
> On Mon, May 24, 2010 at 08:10:16PM +, Blue Swirl wrote:
> > On Mon, May 24, 2010 at 3:40 PM, Joerg Roedel  wrote:
> > >> +
> > >> +#define MMIO_SIZE ? ? ? ? ? ? ? 0x2028
> > >
> > > This size should be a power-of-two value. In this case probably 0x4000.
> > 
> > Not really, the devices can reserve regions of any size. There were
> > some implementation deficiencies in earlier versions of QEMU, where
> > the whole page would be reserved anyway, but this limitation has been
> > removed long time ago.
> 
> The drivers for AMD IOMMU expect that to be 0x4000. At least the Linux
> driver maps the MMIO region with this size. So the emulation should
> reserve this amount of MMIO space too.
> 
>   Joerg

Yeah, I'll change that, since I already reserve 0x4000 bytes in SeaBIOS
for it (I did that to deal with the 16 KiB alignment requirement).


Eduard




Re: [Qemu-devel] Re: [RFC PATCH] AMD IOMMU emulation

2010-05-25 Thread Joerg Roedel
On Mon, May 24, 2010 at 08:10:16PM +, Blue Swirl wrote:
> On Mon, May 24, 2010 at 3:40 PM, Joerg Roedel  wrote:
> >> +
> >> +#define MMIO_SIZE               0x2028
> >
> > This size should be a power-of-two value. In this case probably 0x4000.
> 
> Not really, the devices can reserve regions of any size. There were
> some implementation deficiencies in earlier versions of QEMU, where
> the whole page would be reserved anyway, but this limitation has been
> removed long time ago.

The drivers for AMD IOMMU expect that to be 0x4000. At least the Linux
driver maps the MMIO region with this size. So the emulation should
reserve this amount of MMIO space too.

Joerg




Re: [Qemu-devel] Re: [RFC PATCH] AMD IOMMU emulation

2010-05-24 Thread Blue Swirl
On Mon, May 24, 2010 at 3:40 PM, Joerg Roedel  wrote:
> Hi Eduard,
>
> On Thu, May 20, 2010 at 04:50:07PM +0300, Eduard - Gabriel Munteanu wrote:
>> +  --enable-amd-iommu-emul) amd_iommu="yes"
>> +  ;;
>
> A compile-time option is a good idea.
>
>> +/* MMIO registers */
>> +#define MMIO_DEVICE_TABLE       0x
>> +#define MMIO_COMMAND_BASE       0x0008
>> +#define MMIO_EVENT_BASE         0x0010
>> +#define MMIO_CONTROL            0x0018
>> +#define MMIO_EXCL_BASE          0x0020
>> +#define MMIO_EXCL_LIMIT         0x0028
>> +#define MMIO_COMMAND_HEAD       0x2000
>> +#define MMIO_COMMAND_TAIL       0x2008
>> +#define MMIO_EVENT_HEAD         0x2010
>> +#define MMIO_EVENT_TAIL         0x2018
>> +#define MMIO_STATUS             0x2020
>> +
>> +#define MMIO_SIZE               0x2028
>
> This size should be a power-of-two value. In this case probably 0x4000.

Not really, the devices can reserve regions of any size. There were
some implementation deficiencies in earlier versions of QEMU, where
the whole page would be reserved anyway, but this limitation has been
removed long time ago.

>> +#define MMIO_DEVTAB_SIZE_MASK   ((1UL << 12) - 1)
>> +#define MMIO_DEVTAB_BASE_MASK   (((1UL << 52) - 1) & ~MMIO_DEVTAB_SIZE_MASK)
>
> You must use ULL to be 32bit safe. This is also true for the defines
> below.
>
> [...]
>
> Otherwise the code looks good so far. Seems like the next step should be
> some work on a qemu dma-layer where you can hook the translation into.
>
>        Joerg
>
>
>



[Qemu-devel] Re: [RFC PATCH] AMD IOMMU emulation

2010-05-24 Thread Joerg Roedel
Hi Eduard,

On Thu, May 20, 2010 at 04:50:07PM +0300, Eduard - Gabriel Munteanu wrote:
> +  --enable-amd-iommu-emul) amd_iommu="yes"
> +  ;;

A compile-time option is a good idea.

> +/* MMIO registers */
> +#define MMIO_DEVICE_TABLE   0x
> +#define MMIO_COMMAND_BASE   0x0008
> +#define MMIO_EVENT_BASE 0x0010
> +#define MMIO_CONTROL0x0018
> +#define MMIO_EXCL_BASE  0x0020
> +#define MMIO_EXCL_LIMIT 0x0028
> +#define MMIO_COMMAND_HEAD   0x2000
> +#define MMIO_COMMAND_TAIL   0x2008
> +#define MMIO_EVENT_HEAD 0x2010
> +#define MMIO_EVENT_TAIL 0x2018
> +#define MMIO_STATUS 0x2020
> +
> +#define MMIO_SIZE   0x2028

This size should be a power-of-two value. In this case probably 0x4000.

> +#define MMIO_DEVTAB_SIZE_MASK   ((1UL << 12) - 1)
> +#define MMIO_DEVTAB_BASE_MASK   (((1UL << 52) - 1) & ~MMIO_DEVTAB_SIZE_MASK)

You must use ULL to be 32bit safe. This is also true for the defines
below.

[...]

Otherwise the code looks good so far. Seems like the next step should be
some work on a qemu dma-layer where you can hook the translation into.

Joerg