Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Michael S. Tsirkin]

On Sun, Mar 13, 2016 at 03:14:37AM +0300, David Kiarie wrote:
> On Fri, Mar 11, 2016 at 4:22 PM, Michael S. Tsirkin  wrote:
> > On Sun, Feb 21, 2016 at 09:11:00PM +0300, David Kiarie wrote:
> >> Add AMD IOMMU emulation support to q35 chipset
> >>
> >> Signed-off-by: David Kiarie 
> >> ---
> >>  hw/pci-host/piix.c|  1 +
> >>  hw/pci-host/q35.c | 14 --
> >>  include/hw/i386/intel_iommu.h |  1 +
> >>  3 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >> index 41aa66f..ab2e24a 100644
> >> --- a/hw/pci-host/piix.c
> >> +++ b/hw/pci-host/piix.c
> >> @@ -36,6 +36,7 @@
> >>  #include "hw/i386/ioapic.h"
> >>  #include "qapi/visitor.h"
> >>  #include "qemu/error-report.h"
> >> +#include "hw/i386/amd_iommu.h"
> >>
> >>  /*
> >>   * I440FX chipset data sheet.
> >
> > Why is this needed?
> >
> >> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >> index 115fb8c..355fb32 100644
> >> --- a/hw/pci-host/q35.c
> >> +++ b/hw/pci-host/q35.c
> >> @@ -31,6 +31,7 @@
> >>  #include "hw/hw.h"
> >>  #include "hw/pci-host/q35.h"
> >>  #include "qapi/visitor.h"
> >> +#include "hw/i386/amd_iommu.h"
> >>
> >>  
> >> /
> >>   * Q35 host
> >> @@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
> >>   mch->pci_address_space, >pam_regions[i+1],
> >>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> >>  }
> >> -/* Intel IOMMU (VT-d) */
> >> -if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> >> +
> >> +if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR) == 
> >> 0) {
> >> +/* Intel IOMMU (VT-d) */
> >>  mch_init_dmar(mch);
> >> +} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, 
> >> AMD_IOMMU_STR)
> >> +   == 0) {
> >> +AMDIOMMUState *iommu_state;
> >> +PCIDevice *iommu;
> >> +PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> >> +iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
> >
> > address can be set through a property.
> 
> I missed something here, what is the problem ?Is it the hardcoded address ?

As long as you use pci_create_simple, it's not a problem.
If eventually we manage to switch to using -device,
it'll be useful that by setting address property
machine can influence address for the device.


> >
> >> +iommu_state = AMD_IOMMU_DEVICE(iommu);
> >> +pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
> >
> >
> > It would be better to move this chunk to a separate function.
> >
> >>  }
> >>  }
> >>
> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> >> index b024ffa..539530c 100644
> >> --- a/include/hw/i386/intel_iommu.h
> >> +++ b/include/hw/i386/intel_iommu.h
> >> @@ -27,6 +27,7 @@
> >>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
> >>  #define INTEL_IOMMU_DEVICE(obj) \
> >>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
> >> +#define INTEL_IOMMU_STR "intel"
> >>
> >>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
> >>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL
> >> --
> >> 2.1.4

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[David Kiarie]

On Fri, Mar 11, 2016 at 4:22 PM, Michael S. Tsirkin  wrote:
> On Sun, Feb 21, 2016 at 09:11:00PM +0300, David Kiarie wrote:
>> Add AMD IOMMU emulation support to q35 chipset
>>
>> Signed-off-by: David Kiarie 
>> ---
>>  hw/pci-host/piix.c|  1 +
>>  hw/pci-host/q35.c | 14 --
>>  include/hw/i386/intel_iommu.h |  1 +
>>  3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 41aa66f..ab2e24a 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -36,6 +36,7 @@
>>  #include "hw/i386/ioapic.h"
>>  #include "qapi/visitor.h"
>>  #include "qemu/error-report.h"
>> +#include "hw/i386/amd_iommu.h"
>>
>>  /*
>>   * I440FX chipset data sheet.
>
> Why is this needed?
>
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 115fb8c..355fb32 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/hw.h"
>>  #include "hw/pci-host/q35.h"
>>  #include "qapi/visitor.h"
>> +#include "hw/i386/amd_iommu.h"
>>
>>  
>> /
>>   * Q35 host
>> @@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>   mch->pci_address_space, >pam_regions[i+1],
>>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>>  }
>> -/* Intel IOMMU (VT-d) */
>> -if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>> +
>> +if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR) == 
>> 0) {
>> +/* Intel IOMMU (VT-d) */
>>  mch_init_dmar(mch);
>> +} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, AMD_IOMMU_STR)
>> +   == 0) {
>> +AMDIOMMUState *iommu_state;
>> +PCIDevice *iommu;
>> +PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>> +iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
>
> address can be set through a property.

I missed something here, what is the problem ?Is it the hardcoded address ?

>
>> +iommu_state = AMD_IOMMU_DEVICE(iommu);
>> +pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
>
>
> It would be better to move this chunk to a separate function.
>
>>  }
>>  }
>>
>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>> index b024ffa..539530c 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -27,6 +27,7 @@
>>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
>>  #define INTEL_IOMMU_DEVICE(obj) \
>>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
>> +#define INTEL_IOMMU_STR "intel"
>>
>>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
>>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL
>> --
>> 2.1.4

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Michael S. Tsirkin]

On Sun, Feb 21, 2016 at 09:11:00PM +0300, David Kiarie wrote:
> Add AMD IOMMU emulation support to q35 chipset
> 
> Signed-off-by: David Kiarie 
> ---
>  hw/pci-host/piix.c|  1 +
>  hw/pci-host/q35.c | 14 --
>  include/hw/i386/intel_iommu.h |  1 +
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 41aa66f..ab2e24a 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -36,6 +36,7 @@
>  #include "hw/i386/ioapic.h"
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
> +#include "hw/i386/amd_iommu.h"
>  
>  /*
>   * I440FX chipset data sheet.

Why is this needed?

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 115fb8c..355fb32 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -31,6 +31,7 @@
>  #include "hw/hw.h"
>  #include "hw/pci-host/q35.h"
>  #include "qapi/visitor.h"
> +#include "hw/i386/amd_iommu.h"
>  
>  /
>   * Q35 host
> @@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
>   mch->pci_address_space, >pam_regions[i+1],
>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>  }
> -/* Intel IOMMU (VT-d) */
> -if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> +
> +if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR) == 0) 
> {
> +/* Intel IOMMU (VT-d) */
>  mch_init_dmar(mch);
> +} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, AMD_IOMMU_STR)
> +   == 0) {
> +AMDIOMMUState *iommu_state;
> +PCIDevice *iommu;
> +PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> +iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);

address can be set through a property.

> +iommu_state = AMD_IOMMU_DEVICE(iommu);
> +pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);


It would be better to move this chunk to a separate function.

>  }
>  }
>  
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index b024ffa..539530c 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -27,6 +27,7 @@
>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
>  #define INTEL_IOMMU_DEVICE(obj) \
>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
> +#define INTEL_IOMMU_STR "intel"
>  
>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL
> -- 
> 2.1.4

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[David Kiarie]

On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin  wrote:
> On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:
>> On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin  wrote:
>> > On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:
>> >>
>> >>
>> >> On 22/02/16 14:22, Marcel Apfelbaum wrote:
>> >> >On 02/21/2016 08:11 PM, David Kiarie wrote:
>> >> >>Add AMD IOMMU emulation support to q35 chipset
>> >> >>
>> >> >>Signed-off-by: David Kiarie 
>> >> >>---
>> >> >>  hw/pci-host/piix.c|  1 +
>> >> >>  hw/pci-host/q35.c | 14 --
>> >> >>  include/hw/i386/intel_iommu.h |  1 +
>> >> >>  3 files changed, 14 insertions(+), 2 deletions(-)
>> >> >>
>> >> >>diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> >> >>index 41aa66f..ab2e24a 100644
>> >> >>--- a/hw/pci-host/piix.c
>> >> >>+++ b/hw/pci-host/piix.c
>> >> >>@@ -36,6 +36,7 @@
>> >> >>  #include "hw/i386/ioapic.h"
>> >> >>  #include "qapi/visitor.h"
>> >> >>  #include "qemu/error-report.h"
>> >> >>+#include "hw/i386/amd_iommu.h"
>> >> >
>> >> >Hi,
>> >> >
>> >> >I think you don't need this include anymore.
>> >> >
>> >> >>
>> >> >>  /*
>> >> >>   * I440FX chipset data sheet.
>> >> >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> >> >>index 115fb8c..355fb32 100644
>> >> >>--- a/hw/pci-host/q35.c
>> >> >>+++ b/hw/pci-host/q35.c
>> >> >>@@ -31,6 +31,7 @@
>> >> >>  #include "hw/hw.h"
>> >> >>  #include "hw/pci-host/q35.h"
>> >> >>  #include "qapi/visitor.h"
>> >> >>+#include "hw/i386/amd_iommu.h"
>> >> >>
>> >> >>/
>> >> >>   * Q35 host
>> >> >>@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
>> >> >>   mch->pci_address_space, >pam_regions[i+1],
>> >> >>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>> >> >>  }
>> >> >>-/* Intel IOMMU (VT-d) */
>> >> >>-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>> >> >>+
>> >> >>+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
>> >> >>== 0) {
>> >> >>+/* Intel IOMMU (VT-d) */
>> >> >>  mch_init_dmar(mch);
>> >> >>+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
>> >> >>AMD_IOMMU_STR)
>> >> >>+   == 0) {
>> >> >>+AMDIOMMUState *iommu_state;
>> >> >>+PCIDevice *iommu;
>> >> >>+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>> >> >>+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
>> >
>> > Pls don't hardcode paths like this. Set addr property instead.
>> >
>> >> >>+iommu_state = AMD_IOMMU_DEVICE(iommu);
>> >> >>+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
>> >> >
>> >> >pci_setup_iommu third parameter is void*, so you don't need to cast to
>> >> >AMDIOMMUState
>> >> >before passing it.
>> >>
>> >> This include is necessary for the definition of "AMD_IOMMU_STR" either way
>> >> so am leaving this as is.
>> >
>> > This option parsing is just too ugly.
>> >
>> > Looks like it was a mistake to support the iommu
>> > machine property, but I see no reason to add to the
>> > existing mess.
>> >
>> > Can't users create iommu with -device amd-iommu ?
>>
>> You mean getting rid of the above code and starting device with
>> '-device amd-iommu' ? This way am not able to setup IOMMU regions for
>> devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
>> sets up IOMMU region for IOMMU only. Calling this from the bus sets up
>> IOMMU regions for all devices though.
>>
>> I need to setup IOMMU regions for all devices from the bus, to be able
>> to do that I need to have created the IOMMU device first.
>
> I agree it's unfortunate, but I don't think internal limitations
> should affect the user interface like this.
>
> I wish we had a way to specify initialization order for devices in some
> way, such that system devices are created before others.
>
> For now, can you call pci_setup_iommu in the machine done callback?

Looking at this, machine_done seems to be happening too late. It seems
IOMMU regions are setup by 'pcidev->realize'( at which time you need
to have supplied the bus-IOMMU-regions-setup callback).
pci_iommu_setup is the function that sets up the
bus-IOMMU-regions-setup callback. There could be some other way am
missing but the problem that comes to my head is that if we are not
guaranteed the IOMMU device will be created first. we could still have
problems with some devices having IOMMU regions while others don't.

>
>> >
>> > It's necessary if we are to support multiple IOMMUs, anyway.
>> >
>> >> >
>> >> >Thanks,
>> >> >Marcel
>> >> >
>> >> >>  }
>> >> >>  }
>> >> >>
>> >> >>diff --git a/include/hw/i386/intel_iommu.h
>> >> >>b/include/hw/i386/intel_iommu.h
>> >> >>index b024ffa..539530c 100644
>> >> >>--- a/include/hw/i386/intel_iommu.h
>> >> >>+++ 

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 03:18:18PM +0300, David Kiarie wrote:
> >> Actually we have good news on this front.
> >> You can use as many pxb-pcie devices as you want to create extra PCI root
> >> buses.
> >> Assigning them different IOMMUs would be great. (long term, of course)
> >>
> >
> > Even without pxb-pcie, you can add a pci-bridge to expose a secondary PCI
> > bus
> > and limit IOMMU scope to that PCI bridge.
> 
> Hmmh, good to know. Will look at that.

OK but there's no rush wrt that.
What I chiefly would like is getting command line right
so we don't need to maintain legacy command line syntax.
Support for many iommus can come later,

-- 
MST

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[David Kiarie]

On Thu, Mar 3, 2016 at 3:06 PM, Marcel Apfelbaum  wrote:
> On 03/03/2016 02:02 PM, Marcel Apfelbaum wrote:
>>
>> On 03/03/2016 01:47 PM, David Kiarie wrote:
>>>
>>> On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin 
>>> wrote:

 On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:
>
> On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin 
> wrote:
>>
>> On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:
>>>
>>>
>>>
>>> On 22/02/16 14:22, Marcel Apfelbaum wrote:

 On 02/21/2016 08:11 PM, David Kiarie wrote:
>
> Add AMD IOMMU emulation support to q35 chipset
>
> Signed-off-by: David Kiarie 
> ---
>   hw/pci-host/piix.c|  1 +
>   hw/pci-host/q35.c | 14 --
>   include/hw/i386/intel_iommu.h |  1 +
>   3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 41aa66f..ab2e24a 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -36,6 +36,7 @@
>   #include "hw/i386/ioapic.h"
>   #include "qapi/visitor.h"
>   #include "qemu/error-report.h"
> +#include "hw/i386/amd_iommu.h"


 Hi,

 I think you don't need this include anymore.

>
>   /*
>* I440FX chipset data sheet.
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 115fb8c..355fb32 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -31,6 +31,7 @@
>   #include "hw/hw.h"
>   #include "hw/pci-host/q35.h"
>   #include "qapi/visitor.h"
> +#include "hw/i386/amd_iommu.h"
>
>
> /
>* Q35 host
> @@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error
> **errp)
>mch->pci_address_space, >pam_regions[i+1],
>PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
> PAM_EXPAN_SIZE);
>   }
> -/* Intel IOMMU (VT-d) */
> -if (object_property_get_bool(qdev_get_machine(), "iommu",
> NULL)) {
> +
> +if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
> INTEL_IOMMU_STR)
> == 0) {
> +/* Intel IOMMU (VT-d) */
>   mch_init_dmar(mch);
> +} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
> AMD_IOMMU_STR)
> +   == 0) {
> +AMDIOMMUState *iommu_state;
> +PCIDevice *iommu;
> +PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> +iommu = pci_create_simple(bus, 0x20,
> TYPE_AMD_IOMMU_DEVICE);
>>
>>
>> Pls don't hardcode paths like this. Set addr property instead.
>>
> +iommu_state = AMD_IOMMU_DEVICE(iommu);
> +pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);


 pci_setup_iommu third parameter is void*, so you don't need to cast
 to
 AMDIOMMUState
 before passing it.
>>>
>>>
>>> This include is necessary for the definition of "AMD_IOMMU_STR"
>>> either way
>>> so am leaving this as is.
>>
>>
>> This option parsing is just too ugly.
>>
>> Looks like it was a mistake to support the iommu
>> machine property, but I see no reason to add to the
>> existing mess.
>>
>> Can't users create iommu with -device amd-iommu ?
>
>
> You mean getting rid of the above code and starting device with
> '-device amd-iommu' ? This way am not able to setup IOMMU regions for
> devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
> sets up IOMMU region for IOMMU only. Calling this from the bus sets up
> IOMMU regions for all devices though.
>
> I need to setup IOMMU regions for all devices from the bus, to be able
> to do that I need to have created the IOMMU device first.


 I agree it's unfortunate, but I don't think internal limitations
 should affect the user interface like this.

 I wish we had a way to specify initialization order for devices in some
 way, such that system devices are created before others.

 For now, can you call pci_setup_iommu in the machine done callback?

>>
>> It's necessary if we are to support multiple IOMMUs, anyway.
>>>
>>>
>>> If this is mainly to allow users run multiple IOMMUs I think the
>>> greatest limitation is that Qemu only have one PCI bus which can only
>>> host one IOMMU, AFAIK so unless there are huge structural changes
>>> running 

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Marcel Apfelbaum]

On 03/03/2016 02:02 PM, Marcel Apfelbaum wrote:

On 03/03/2016 01:47 PM, David Kiarie wrote:

On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin  wrote:

On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:

On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin  wrote:

On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:



On 22/02/16 14:22, Marcel Apfelbaum wrote:

On 02/21/2016 08:11 PM, David Kiarie wrote:

Add AMD IOMMU emulation support to q35 chipset

Signed-off-by: David Kiarie 
---
  hw/pci-host/piix.c|  1 +
  hw/pci-host/q35.c | 14 --
  include/hw/i386/intel_iommu.h |  1 +
  3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 41aa66f..ab2e24a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -36,6 +36,7 @@
  #include "hw/i386/ioapic.h"
  #include "qapi/visitor.h"
  #include "qemu/error-report.h"
+#include "hw/i386/amd_iommu.h"


Hi,

I think you don't need this include anymore.



  /*
   * I440FX chipset data sheet.
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 115fb8c..355fb32 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -31,6 +31,7 @@
  #include "hw/hw.h"
  #include "hw/pci-host/q35.h"
  #include "qapi/visitor.h"
+#include "hw/i386/amd_iommu.h"

/
   * Q35 host
@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
   mch->pci_address_space, >pam_regions[i+1],
   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
  }
-/* Intel IOMMU (VT-d) */
-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
+
+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
== 0) {
+/* Intel IOMMU (VT-d) */
  mch_init_dmar(mch);
+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
AMD_IOMMU_STR)
+   == 0) {
+AMDIOMMUState *iommu_state;
+PCIDevice *iommu;
+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);


Pls don't hardcode paths like this. Set addr property instead.


+iommu_state = AMD_IOMMU_DEVICE(iommu);
+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);


pci_setup_iommu third parameter is void*, so you don't need to cast to
AMDIOMMUState
before passing it.


This include is necessary for the definition of "AMD_IOMMU_STR" either way
so am leaving this as is.


This option parsing is just too ugly.

Looks like it was a mistake to support the iommu
machine property, but I see no reason to add to the
existing mess.

Can't users create iommu with -device amd-iommu ?


You mean getting rid of the above code and starting device with
'-device amd-iommu' ? This way am not able to setup IOMMU regions for
devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
sets up IOMMU region for IOMMU only. Calling this from the bus sets up
IOMMU regions for all devices though.

I need to setup IOMMU regions for all devices from the bus, to be able
to do that I need to have created the IOMMU device first.


I agree it's unfortunate, but I don't think internal limitations
should affect the user interface like this.

I wish we had a way to specify initialization order for devices in some
way, such that system devices are created before others.

For now, can you call pci_setup_iommu in the machine done callback?



It's necessary if we are to support multiple IOMMUs, anyway.


If this is mainly to allow users run multiple IOMMUs I think the
greatest limitation is that Qemu only have one PCI bus which can only
host one IOMMU, AFAIK so unless there are huge structural changes
running multiple IOMMU on Qemu may not be possible.


Actually we have good news on this front.
You can use as many pxb-pcie devices as you want to create extra PCI root buses.
Assigning them different IOMMUs would be great. (long term, of course)



Even without pxb-pcie, you can add a pci-bridge to expose a secondary PCI bus
and limit IOMMU scope to that PCI bridge.

Thanks,
Marcel



Thanks,
Marcel







Thanks,
Marcel


  }
  }

diff --git a/include/hw/i386/intel_iommu.h
b/include/hw/i386/intel_iommu.h
index b024ffa..539530c 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -27,6 +27,7 @@
  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
  #define INTEL_IOMMU_DEVICE(obj) \
   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
+#define INTEL_IOMMU_STR "intel"

  /* DMAR Hardware Unit Definition address (IOMMU unit) */
  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Marcel Apfelbaum]

On 03/03/2016 01:47 PM, David Kiarie wrote:

On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin  wrote:

On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:

On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin  wrote:

On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:



On 22/02/16 14:22, Marcel Apfelbaum wrote:

On 02/21/2016 08:11 PM, David Kiarie wrote:

Add AMD IOMMU emulation support to q35 chipset

Signed-off-by: David Kiarie 
---
  hw/pci-host/piix.c|  1 +
  hw/pci-host/q35.c | 14 --
  include/hw/i386/intel_iommu.h |  1 +
  3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 41aa66f..ab2e24a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -36,6 +36,7 @@
  #include "hw/i386/ioapic.h"
  #include "qapi/visitor.h"
  #include "qemu/error-report.h"
+#include "hw/i386/amd_iommu.h"


Hi,

I think you don't need this include anymore.



  /*
   * I440FX chipset data sheet.
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 115fb8c..355fb32 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -31,6 +31,7 @@
  #include "hw/hw.h"
  #include "hw/pci-host/q35.h"
  #include "qapi/visitor.h"
+#include "hw/i386/amd_iommu.h"

/
   * Q35 host
@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
   mch->pci_address_space, >pam_regions[i+1],
   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
  }
-/* Intel IOMMU (VT-d) */
-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
+
+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
== 0) {
+/* Intel IOMMU (VT-d) */
  mch_init_dmar(mch);
+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
AMD_IOMMU_STR)
+   == 0) {
+AMDIOMMUState *iommu_state;
+PCIDevice *iommu;
+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);


Pls don't hardcode paths like this. Set addr property instead.


+iommu_state = AMD_IOMMU_DEVICE(iommu);
+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);


pci_setup_iommu third parameter is void*, so you don't need to cast to
AMDIOMMUState
before passing it.


This include is necessary for the definition of "AMD_IOMMU_STR" either way
so am leaving this as is.


This option parsing is just too ugly.

Looks like it was a mistake to support the iommu
machine property, but I see no reason to add to the
existing mess.

Can't users create iommu with -device amd-iommu ?


You mean getting rid of the above code and starting device with
'-device amd-iommu' ? This way am not able to setup IOMMU regions for
devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
sets up IOMMU region for IOMMU only. Calling this from the bus sets up
IOMMU regions for all devices though.

I need to setup IOMMU regions for all devices from the bus, to be able
to do that I need to have created the IOMMU device first.


I agree it's unfortunate, but I don't think internal limitations
should affect the user interface like this.

I wish we had a way to specify initialization order for devices in some
way, such that system devices are created before others.

For now, can you call pci_setup_iommu in the machine done callback?



It's necessary if we are to support multiple IOMMUs, anyway.


If this is mainly to allow users run multiple IOMMUs I think the
greatest limitation is that Qemu only have one PCI bus which can only
host one IOMMU, AFAIK so unless there are huge structural changes
running multiple IOMMU on Qemu may not be possible.


Actually we have good news on this front.
You can use as many pxb-pcie devices as you want to create extra PCI root buses.
Assigning them different IOMMUs would be great. (long term, of course)


Thanks,
Marcel







Thanks,
Marcel


  }
  }

diff --git a/include/hw/i386/intel_iommu.h
b/include/hw/i386/intel_iommu.h
index b024ffa..539530c 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -27,6 +27,7 @@
  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
  #define INTEL_IOMMU_DEVICE(obj) \
   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
+#define INTEL_IOMMU_STR "intel"

  /* DMAR Hardware Unit Definition address (IOMMU unit) */
  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[David Kiarie]

On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin  wrote:
> On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:
>> On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin  wrote:
>> > On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:
>> >>
>> >>
>> >> On 22/02/16 14:22, Marcel Apfelbaum wrote:
>> >> >On 02/21/2016 08:11 PM, David Kiarie wrote:
>> >> >>Add AMD IOMMU emulation support to q35 chipset
>> >> >>
>> >> >>Signed-off-by: David Kiarie 
>> >> >>---
>> >> >>  hw/pci-host/piix.c|  1 +
>> >> >>  hw/pci-host/q35.c | 14 --
>> >> >>  include/hw/i386/intel_iommu.h |  1 +
>> >> >>  3 files changed, 14 insertions(+), 2 deletions(-)
>> >> >>
>> >> >>diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> >> >>index 41aa66f..ab2e24a 100644
>> >> >>--- a/hw/pci-host/piix.c
>> >> >>+++ b/hw/pci-host/piix.c
>> >> >>@@ -36,6 +36,7 @@
>> >> >>  #include "hw/i386/ioapic.h"
>> >> >>  #include "qapi/visitor.h"
>> >> >>  #include "qemu/error-report.h"
>> >> >>+#include "hw/i386/amd_iommu.h"
>> >> >
>> >> >Hi,
>> >> >
>> >> >I think you don't need this include anymore.
>> >> >
>> >> >>
>> >> >>  /*
>> >> >>   * I440FX chipset data sheet.
>> >> >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> >> >>index 115fb8c..355fb32 100644
>> >> >>--- a/hw/pci-host/q35.c
>> >> >>+++ b/hw/pci-host/q35.c
>> >> >>@@ -31,6 +31,7 @@
>> >> >>  #include "hw/hw.h"
>> >> >>  #include "hw/pci-host/q35.h"
>> >> >>  #include "qapi/visitor.h"
>> >> >>+#include "hw/i386/amd_iommu.h"
>> >> >>
>> >> >>/
>> >> >>   * Q35 host
>> >> >>@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
>> >> >>   mch->pci_address_space, >pam_regions[i+1],
>> >> >>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>> >> >>  }
>> >> >>-/* Intel IOMMU (VT-d) */
>> >> >>-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>> >> >>+
>> >> >>+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
>> >> >>== 0) {
>> >> >>+/* Intel IOMMU (VT-d) */
>> >> >>  mch_init_dmar(mch);
>> >> >>+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
>> >> >>AMD_IOMMU_STR)
>> >> >>+   == 0) {
>> >> >>+AMDIOMMUState *iommu_state;
>> >> >>+PCIDevice *iommu;
>> >> >>+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>> >> >>+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
>> >
>> > Pls don't hardcode paths like this. Set addr property instead.
>> >
>> >> >>+iommu_state = AMD_IOMMU_DEVICE(iommu);
>> >> >>+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
>> >> >
>> >> >pci_setup_iommu third parameter is void*, so you don't need to cast to
>> >> >AMDIOMMUState
>> >> >before passing it.
>> >>
>> >> This include is necessary for the definition of "AMD_IOMMU_STR" either way
>> >> so am leaving this as is.
>> >
>> > This option parsing is just too ugly.
>> >
>> > Looks like it was a mistake to support the iommu
>> > machine property, but I see no reason to add to the
>> > existing mess.
>> >
>> > Can't users create iommu with -device amd-iommu ?
>>
>> You mean getting rid of the above code and starting device with
>> '-device amd-iommu' ? This way am not able to setup IOMMU regions for
>> devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
>> sets up IOMMU region for IOMMU only. Calling this from the bus sets up
>> IOMMU regions for all devices though.
>>
>> I need to setup IOMMU regions for all devices from the bus, to be able
>> to do that I need to have created the IOMMU device first.
>
> I agree it's unfortunate, but I don't think internal limitations
> should affect the user interface like this.
>
> I wish we had a way to specify initialization order for devices in some
> way, such that system devices are created before others.
>
> For now, can you call pci_setup_iommu in the machine done callback?
>
>> >
>> > It's necessary if we are to support multiple IOMMUs, anyway.

If this is mainly to allow users run multiple IOMMUs I think the
greatest limitation is that Qemu only have one PCI bus which can only
host one IOMMU, AFAIK so unless there are huge structural changes
running multiple IOMMU on Qemu may not be possible.

>> >
>> >> >
>> >> >Thanks,
>> >> >Marcel
>> >> >
>> >> >>  }
>> >> >>  }
>> >> >>
>> >> >>diff --git a/include/hw/i386/intel_iommu.h
>> >> >>b/include/hw/i386/intel_iommu.h
>> >> >>index b024ffa..539530c 100644
>> >> >>--- a/include/hw/i386/intel_iommu.h
>> >> >>+++ b/include/hw/i386/intel_iommu.h
>> >> >>@@ -27,6 +27,7 @@
>> >> >>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
>> >> >>  #define INTEL_IOMMU_DEVICE(obj) \
>> >> >>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
>> >> >>+#define INTEL_IOMMU_STR "intel"
>> >> >>
>> >> 

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:
> On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin  wrote:
> > On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:
> >>
> >>
> >> On 22/02/16 14:22, Marcel Apfelbaum wrote:
> >> >On 02/21/2016 08:11 PM, David Kiarie wrote:
> >> >>Add AMD IOMMU emulation support to q35 chipset
> >> >>
> >> >>Signed-off-by: David Kiarie 
> >> >>---
> >> >>  hw/pci-host/piix.c|  1 +
> >> >>  hw/pci-host/q35.c | 14 --
> >> >>  include/hw/i386/intel_iommu.h |  1 +
> >> >>  3 files changed, 14 insertions(+), 2 deletions(-)
> >> >>
> >> >>diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >> >>index 41aa66f..ab2e24a 100644
> >> >>--- a/hw/pci-host/piix.c
> >> >>+++ b/hw/pci-host/piix.c
> >> >>@@ -36,6 +36,7 @@
> >> >>  #include "hw/i386/ioapic.h"
> >> >>  #include "qapi/visitor.h"
> >> >>  #include "qemu/error-report.h"
> >> >>+#include "hw/i386/amd_iommu.h"
> >> >
> >> >Hi,
> >> >
> >> >I think you don't need this include anymore.
> >> >
> >> >>
> >> >>  /*
> >> >>   * I440FX chipset data sheet.
> >> >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >> >>index 115fb8c..355fb32 100644
> >> >>--- a/hw/pci-host/q35.c
> >> >>+++ b/hw/pci-host/q35.c
> >> >>@@ -31,6 +31,7 @@
> >> >>  #include "hw/hw.h"
> >> >>  #include "hw/pci-host/q35.h"
> >> >>  #include "qapi/visitor.h"
> >> >>+#include "hw/i386/amd_iommu.h"
> >> >>
> >> >>/
> >> >>   * Q35 host
> >> >>@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
> >> >>   mch->pci_address_space, >pam_regions[i+1],
> >> >>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> >> >>  }
> >> >>-/* Intel IOMMU (VT-d) */
> >> >>-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> >> >>+
> >> >>+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
> >> >>== 0) {
> >> >>+/* Intel IOMMU (VT-d) */
> >> >>  mch_init_dmar(mch);
> >> >>+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
> >> >>AMD_IOMMU_STR)
> >> >>+   == 0) {
> >> >>+AMDIOMMUState *iommu_state;
> >> >>+PCIDevice *iommu;
> >> >>+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> >> >>+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
> >
> > Pls don't hardcode paths like this. Set addr property instead.
> >
> >> >>+iommu_state = AMD_IOMMU_DEVICE(iommu);
> >> >>+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
> >> >
> >> >pci_setup_iommu third parameter is void*, so you don't need to cast to
> >> >AMDIOMMUState
> >> >before passing it.
> >>
> >> This include is necessary for the definition of "AMD_IOMMU_STR" either way
> >> so am leaving this as is.
> >
> > This option parsing is just too ugly.
> >
> > Looks like it was a mistake to support the iommu
> > machine property, but I see no reason to add to the
> > existing mess.
> >
> > Can't users create iommu with -device amd-iommu ?
> 
> You mean getting rid of the above code and starting device with
> '-device amd-iommu' ? This way am not able to setup IOMMU regions for
> devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
> sets up IOMMU region for IOMMU only. Calling this from the bus sets up
> IOMMU regions for all devices though.
> 
> I need to setup IOMMU regions for all devices from the bus, to be able
> to do that I need to have created the IOMMU device first.

I agree it's unfortunate, but I don't think internal limitations
should affect the user interface like this.

I wish we had a way to specify initialization order for devices in some
way, such that system devices are created before others.

For now, can you call pci_setup_iommu in the machine done callback?

> >
> > It's necessary if we are to support multiple IOMMUs, anyway.
> >
> >> >
> >> >Thanks,
> >> >Marcel
> >> >
> >> >>  }
> >> >>  }
> >> >>
> >> >>diff --git a/include/hw/i386/intel_iommu.h
> >> >>b/include/hw/i386/intel_iommu.h
> >> >>index b024ffa..539530c 100644
> >> >>--- a/include/hw/i386/intel_iommu.h
> >> >>+++ b/include/hw/i386/intel_iommu.h
> >> >>@@ -27,6 +27,7 @@
> >> >>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
> >> >>  #define INTEL_IOMMU_DEVICE(obj) \
> >> >>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
> >> >>+#define INTEL_IOMMU_STR "intel"
> >> >>
> >> >>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
> >> >>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL
> >> >>
> >> >

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[David Kiarie]

On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin  wrote:
> On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:
>>
>>
>> On 22/02/16 14:22, Marcel Apfelbaum wrote:
>> >On 02/21/2016 08:11 PM, David Kiarie wrote:
>> >>Add AMD IOMMU emulation support to q35 chipset
>> >>
>> >>Signed-off-by: David Kiarie 
>> >>---
>> >>  hw/pci-host/piix.c|  1 +
>> >>  hw/pci-host/q35.c | 14 --
>> >>  include/hw/i386/intel_iommu.h |  1 +
>> >>  3 files changed, 14 insertions(+), 2 deletions(-)
>> >>
>> >>diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> >>index 41aa66f..ab2e24a 100644
>> >>--- a/hw/pci-host/piix.c
>> >>+++ b/hw/pci-host/piix.c
>> >>@@ -36,6 +36,7 @@
>> >>  #include "hw/i386/ioapic.h"
>> >>  #include "qapi/visitor.h"
>> >>  #include "qemu/error-report.h"
>> >>+#include "hw/i386/amd_iommu.h"
>> >
>> >Hi,
>> >
>> >I think you don't need this include anymore.
>> >
>> >>
>> >>  /*
>> >>   * I440FX chipset data sheet.
>> >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> >>index 115fb8c..355fb32 100644
>> >>--- a/hw/pci-host/q35.c
>> >>+++ b/hw/pci-host/q35.c
>> >>@@ -31,6 +31,7 @@
>> >>  #include "hw/hw.h"
>> >>  #include "hw/pci-host/q35.h"
>> >>  #include "qapi/visitor.h"
>> >>+#include "hw/i386/amd_iommu.h"
>> >>
>> >>/
>> >>   * Q35 host
>> >>@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
>> >>   mch->pci_address_space, >pam_regions[i+1],
>> >>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>> >>  }
>> >>-/* Intel IOMMU (VT-d) */
>> >>-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>> >>+
>> >>+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
>> >>== 0) {
>> >>+/* Intel IOMMU (VT-d) */
>> >>  mch_init_dmar(mch);
>> >>+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
>> >>AMD_IOMMU_STR)
>> >>+   == 0) {
>> >>+AMDIOMMUState *iommu_state;
>> >>+PCIDevice *iommu;
>> >>+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>> >>+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
>
> Pls don't hardcode paths like this. Set addr property instead.
>
>> >>+iommu_state = AMD_IOMMU_DEVICE(iommu);
>> >>+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
>> >
>> >pci_setup_iommu third parameter is void*, so you don't need to cast to
>> >AMDIOMMUState
>> >before passing it.
>>
>> This include is necessary for the definition of "AMD_IOMMU_STR" either way
>> so am leaving this as is.
>
> This option parsing is just too ugly.
>
> Looks like it was a mistake to support the iommu
> machine property, but I see no reason to add to the
> existing mess.
>
> Can't users create iommu with -device amd-iommu ?

You mean getting rid of the above code and starting device with
'-device amd-iommu' ? This way am not able to setup IOMMU regions for
devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
sets up IOMMU region for IOMMU only. Calling this from the bus sets up
IOMMU regions for all devices though.

I need to setup IOMMU regions for all devices from the bus, to be able
to do that I need to have created the IOMMU device first.

>
> It's necessary if we are to support multiple IOMMUs, anyway.
>
>> >
>> >Thanks,
>> >Marcel
>> >
>> >>  }
>> >>  }
>> >>
>> >>diff --git a/include/hw/i386/intel_iommu.h
>> >>b/include/hw/i386/intel_iommu.h
>> >>index b024ffa..539530c 100644
>> >>--- a/include/hw/i386/intel_iommu.h
>> >>+++ b/include/hw/i386/intel_iommu.h
>> >>@@ -27,6 +27,7 @@
>> >>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
>> >>  #define INTEL_IOMMU_DEVICE(obj) \
>> >>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
>> >>+#define INTEL_IOMMU_STR "intel"
>> >>
>> >>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
>> >>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL
>> >>
>> >

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:
> 
> 
> On 22/02/16 14:22, Marcel Apfelbaum wrote:
> >On 02/21/2016 08:11 PM, David Kiarie wrote:
> >>Add AMD IOMMU emulation support to q35 chipset
> >>
> >>Signed-off-by: David Kiarie 
> >>---
> >>  hw/pci-host/piix.c|  1 +
> >>  hw/pci-host/q35.c | 14 --
> >>  include/hw/i386/intel_iommu.h |  1 +
> >>  3 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >>index 41aa66f..ab2e24a 100644
> >>--- a/hw/pci-host/piix.c
> >>+++ b/hw/pci-host/piix.c
> >>@@ -36,6 +36,7 @@
> >>  #include "hw/i386/ioapic.h"
> >>  #include "qapi/visitor.h"
> >>  #include "qemu/error-report.h"
> >>+#include "hw/i386/amd_iommu.h"
> >
> >Hi,
> >
> >I think you don't need this include anymore.
> >
> >>
> >>  /*
> >>   * I440FX chipset data sheet.
> >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >>index 115fb8c..355fb32 100644
> >>--- a/hw/pci-host/q35.c
> >>+++ b/hw/pci-host/q35.c
> >>@@ -31,6 +31,7 @@
> >>  #include "hw/hw.h"
> >>  #include "hw/pci-host/q35.h"
> >>  #include "qapi/visitor.h"
> >>+#include "hw/i386/amd_iommu.h"
> >>
> >>/
> >>   * Q35 host
> >>@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
> >>   mch->pci_address_space, >pam_regions[i+1],
> >>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> >>  }
> >>-/* Intel IOMMU (VT-d) */
> >>-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> >>+
> >>+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
> >>== 0) {
> >>+/* Intel IOMMU (VT-d) */
> >>  mch_init_dmar(mch);
> >>+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
> >>AMD_IOMMU_STR)
> >>+   == 0) {
> >>+AMDIOMMUState *iommu_state;
> >>+PCIDevice *iommu;
> >>+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> >>+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);

Pls don't hardcode paths like this. Set addr property instead.

> >>+iommu_state = AMD_IOMMU_DEVICE(iommu);
> >>+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);
> >
> >pci_setup_iommu third parameter is void*, so you don't need to cast to
> >AMDIOMMUState
> >before passing it.
> 
> This include is necessary for the definition of "AMD_IOMMU_STR" either way
> so am leaving this as is.

This option parsing is just too ugly.

Looks like it was a mistake to support the iommu
machine property, but I see no reason to add to the
existing mess.

Can't users create iommu with -device amd-iommu ?

It's necessary if we are to support multiple IOMMUs, anyway.

> >
> >Thanks,
> >Marcel
> >
> >>  }
> >>  }
> >>
> >>diff --git a/include/hw/i386/intel_iommu.h
> >>b/include/hw/i386/intel_iommu.h
> >>index b024ffa..539530c 100644
> >>--- a/include/hw/i386/intel_iommu.h
> >>+++ b/include/hw/i386/intel_iommu.h
> >>@@ -27,6 +27,7 @@
> >>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
> >>  #define INTEL_IOMMU_DEVICE(obj) \
> >>   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
> >>+#define INTEL_IOMMU_STR "intel"
> >>
> >>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
> >>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL
> >>
> >

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

[Marcel Apfelbaum]

On 02/21/2016 08:11 PM, David Kiarie wrote:

Add AMD IOMMU emulation support to q35 chipset

Signed-off-by: David Kiarie 
---
  hw/pci-host/piix.c|  1 +
  hw/pci-host/q35.c | 14 --
  include/hw/i386/intel_iommu.h |  1 +
  3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 41aa66f..ab2e24a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -36,6 +36,7 @@
  #include "hw/i386/ioapic.h"
  #include "qapi/visitor.h"
  #include "qemu/error-report.h"
+#include "hw/i386/amd_iommu.h"


Hi,

I think you don't need this include anymore.



  /*
   * I440FX chipset data sheet.
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 115fb8c..355fb32 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -31,6 +31,7 @@
  #include "hw/hw.h"
  #include "hw/pci-host/q35.h"
  #include "qapi/visitor.h"
+#include "hw/i386/amd_iommu.h"

  /
   * Q35 host
@@ -505,9 +506,18 @@ static void mch_realize(PCIDevice *d, Error **errp)
   mch->pci_address_space, >pam_regions[i+1],
   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
  }
-/* Intel IOMMU (VT-d) */
-if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
+
+if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR) == 0) {
+/* Intel IOMMU (VT-d) */
  mch_init_dmar(mch);
+} else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, AMD_IOMMU_STR)
+   == 0) {
+AMDIOMMUState *iommu_state;
+PCIDevice *iommu;
+PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
+iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);
+iommu_state = AMD_IOMMU_DEVICE(iommu);
+pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);


pci_setup_iommu third parameter is void*, so you don't need to cast to 
AMDIOMMUState
before passing it.

Thanks,
Marcel


  }
  }

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b024ffa..539530c 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -27,6 +27,7 @@
  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
  #define INTEL_IOMMU_DEVICE(obj) \
   OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
+#define INTEL_IOMMU_STR "intel"

  /* DMAR Hardware Unit Definition address (IOMMU unit) */
  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed9ULL