[Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Peter Xu]

Intel vIOMMU devices are created with "-device" parameter, while here
actually we need to make sure the dmar device be created before other
PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
will be setup correctly before realizations of those PCI devices (it is
legal that PCI device fetch these info during its realization). Now this
ordering yet cannot be achieved elsewhere, and devices will be created
in the order that user specified. We need to avoid that.

This patch tries to detect this kind of misordering issue during init of
VT-d device, then report to guest if misordering happened. In the
future, we can provide something better to solve it, e.g., to support
device init ordering, then we can live without this patch.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
v3: added virtio-pci device detection since we have that requirement as
    well now.

PS. this patch should be needed along with Jason's:
 "virtio: unbreak virtio-pci with IOMMU after caching ring translations"
to make sure virtio devices with vt-d are safe for 2.9.
---
 hw/i386/intel_iommu.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 22d8226..1077f90 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
     return true;
 }
 
+/*
+ * TODO: we should have a better way to achieve the ordering rather
+ * than this misorder check explicitly against vfio-pci and virtio-pci
+ * devices. Here, there's no easy way to detect init of virtio-pci
+ * devices, instead we detect the virtio bus.
+ */
+static bool vtd_detected_misorder_init(Error **errp)
+{
+    if (object_resolve_path_type("", "vfio-pci", NULL) ||
+        object_resolve_path_type("", "virtio-pci-bus", NULL)) {
+        error_setg(errp, "Please specify \"intel-iommu\" before "
+                   "all the rest of the devices.");
+        return true;
+    }
+
+    return false;
+}
+
 static void vtd_realize(DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
@@ -2567,6 +2585,10 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
 
+    if (vtd_detected_misorder_init(errp)) {
+        return;
+    }
+
     VTD_DPRINTF(GENERAL, "");
     x86_iommu->type = TYPE_INTEL;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Michael S. Tsirkin]

On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
> Intel vIOMMU devices are created with "-device" parameter, while here
> actually we need to make sure the dmar device be created before other
> PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
> will be setup correctly before realizations of those PCI devices (it is
> legal that PCI device fetch these info during its realization). Now this
> ordering yet cannot be achieved elsewhere, and devices will be created
> in the order that user specified. We need to avoid that.
> 
> This patch tries to detect this kind of misordering issue during init of
> VT-d device, then report to guest if misordering happened. In the
> future, we can provide something better to solve it, e.g., to support
> device init ordering, then we can live without this patch.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Unfortunately with virtio it's a regression, as it used to
work with iommu. So I'm afraid we need to look into supporting
arbitrary order right now :(

> ---
> v3: added virtio-pci device detection since we have that requirement as
>     well now.
> 
> PS. this patch should be needed along with Jason's:
>  "virtio: unbreak virtio-pci with IOMMU after caching ring translations"
> to make sure virtio devices with vt-d are safe for 2.9.
> ---
>  hw/i386/intel_iommu.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 22d8226..1077f90 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>      return true;
>  }
>  
> +/*
> + * TODO: we should have a better way to achieve the ordering rather
> + * than this misorder check explicitly against vfio-pci and virtio-pci
> + * devices. Here, there's no easy way to detect init of virtio-pci
> + * devices, instead we detect the virtio bus.
> + */
> +static bool vtd_detected_misorder_init(Error **errp)
> +{
> +    if (object_resolve_path_type("", "vfio-pci", NULL) ||
> +        object_resolve_path_type("", "virtio-pci-bus", NULL)) {
> +        error_setg(errp, "Please specify \"intel-iommu\" before "
> +                   "all the rest of the devices.");
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());

We'd put this somewhere central - this hack is not vtd specific.



> @@ -2567,6 +2585,10 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>  
> +    if (vtd_detected_misorder_init(errp)) {
> +        return;
> +    }
> +
>      VTD_DPRINTF(GENERAL, "");
>      x86_iommu->type = TYPE_INTEL;
>  
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Peter Xu]

On Thu, Mar 02, 2017 at 07:13:08AM +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
> > Intel vIOMMU devices are created with "-device" parameter, while here
> > actually we need to make sure the dmar device be created before other
> > PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
> > will be setup correctly before realizations of those PCI devices (it is
> > legal that PCI device fetch these info during its realization). Now this
> > ordering yet cannot be achieved elsewhere, and devices will be created
> > in the order that user specified. We need to avoid that.
> > 
> > This patch tries to detect this kind of misordering issue during init of
> > VT-d device, then report to guest if misordering happened. In the
> > future, we can provide something better to solve it, e.g., to support
> > device init ordering, then we can live without this patch.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Unfortunately with virtio it's a regression, as it used to
> work with iommu. So I'm afraid we need to look into supporting
> arbitrary order right now :(

No problem.

But do you think that would be a possible material for 2.9?

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Michael S. Tsirkin]

On Thu, Mar 02, 2017 at 01:20:53PM +0800, Peter Xu wrote:
> On Thu, Mar 02, 2017 at 07:13:08AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
> > > Intel vIOMMU devices are created with "-device" parameter, while here
> > > actually we need to make sure the dmar device be created before other
> > > PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
> > > will be setup correctly before realizations of those PCI devices (it is
> > > legal that PCI device fetch these info during its realization). Now this
> > > ordering yet cannot be achieved elsewhere, and devices will be created
> > > in the order that user specified. We need to avoid that.
> > > 
> > > This patch tries to detect this kind of misordering issue during init of
> > > VT-d device, then report to guest if misordering happened. In the
> > > future, we can provide something better to solve it, e.g., to support
> > > device init ordering, then we can live without this patch.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > Unfortunately with virtio it's a regression, as it used to
> > work with iommu. So I'm afraid we need to look into supporting
> > arbitrary order right now :(
> 
> No problem.
> 
> But do you think that would be a possible material for 2.9?
> 
> Thanks,
> 
> -- peterx

Looks like it's that or revert the virtio caching feature?

-- 
MST

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Marcel Apfelbaum]

On 03/02/2017 07:13 AM, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
>> Intel vIOMMU devices are created with "-device" parameter, while here
>> actually we need to make sure the dmar device be created before other
>> PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
>> will be setup correctly before realizations of those PCI devices (it is
>> legal that PCI device fetch these info during its realization). Now this
>> ordering yet cannot be achieved elsewhere, and devices will be created
>> in the order that user specified. We need to avoid that.
>>
>> This patch tries to detect this kind of misordering issue during init of
>> VT-d device, then report to guest if misordering happened. In the
>> future, we can provide something better to solve it, e.g., to support
>> device init ordering, then we can live without this patch.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>
> Unfortunately with virtio it's a regression, as it used to
> work with iommu. So I'm afraid we need to look into supporting
> arbitrary order right now :(
>

Hi,

A fast way to do it is to initialize iommu with a new keyword, like
     -iommu intel-iommu
Or maybe use the existing "-object" somehow ?
 From vl.c comments:
   "Initial object creation happens before all other
    QEMU data types are created.... "

Another idea is to add a third run on QEMU cmd line arguments,
but this would affect the whole system.

However having an ordering system beats all other ideas.

Thanks,
Marcel

>> ---
>> v3: added virtio-pci device detection since we have that requirement as
>>     well now.
>>
>> PS. this patch should be needed along with Jason's:
>>  "virtio: unbreak virtio-pci with IOMMU after caching ring translations"
>> to make sure virtio devices with vt-d are safe for 2.9.
>> ---
>>  hw/i386/intel_iommu.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 22d8226..1077f90 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>      return true;
>>  }
>>
>> +/*
>> + * TODO: we should have a better way to achieve the ordering rather
>> + * than this misorder check explicitly against vfio-pci and virtio-pci
>> + * devices. Here, there's no easy way to detect init of virtio-pci
>> + * devices, instead we detect the virtio bus.
>> + */
>> +static bool vtd_detected_misorder_init(Error **errp)
>> +{
>> +    if (object_resolve_path_type("", "vfio-pci", NULL) ||
>> +        object_resolve_path_type("", "virtio-pci-bus", NULL)) {
>> +        error_setg(errp, "Please specify \"intel-iommu\" before "
>> +                   "all the rest of the devices.");
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>  static void vtd_realize(DeviceState *dev, Error **errp)
>>  {
>>      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>
> We'd put this somewhere central - this hack is not vtd specific.
>
>
>
>> @@ -2567,6 +2585,10 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>>
>> +    if (vtd_detected_misorder_init(errp)) {
>> +        return;
>> +    }
>> +
>>      VTD_DPRINTF(GENERAL, "");
>>      x86_iommu->type = TYPE_INTEL;
>>
>> --
>> 2.7.4

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Paolo Bonzini]

----- Original Message -----
> From: "Marcel Apfelbaum" <marcel@redhat.com>
> To: "Michael S. Tsirkin" <mst@redhat.com>, "Peter Xu" <peterx@redhat.com>
> Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>, "yi l liu" <yi.l.liu@intel.com>, "Jintack Lim"
> <jintack@cs.columbia.edu>, "Jason Wang" <jasowang@redhat.com>, "Alex Williamson" <alex.williamson@redhat.com>
> Sent: Thursday, March 2, 2017 7:24:44 AM
> Subject: Re: [PATCH v3] intel_iommu: check misordered init when realize
> 
> On 03/02/2017 07:13 AM, Michael S. Tsirkin wrote:
> > On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
> >> Intel vIOMMU devices are created with "-device" parameter, while here
> >> actually we need to make sure the dmar device be created before other
> >> PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
> >> will be setup correctly before realizations of those PCI devices (it is
> >> legal that PCI device fetch these info during its realization). Now this
> >> ordering yet cannot be achieved elsewhere, and devices will be created
> >> in the order that user specified. We need to avoid that.
> >>
> >> This patch tries to detect this kind of misordering issue during init of
> >> VT-d device, then report to guest if misordering happened. In the
> >> future, we can provide something better to solve it, e.g., to support
> >> device init ordering, then we can live without this patch.
> >>
> >> Signed-off-by: Peter Xu <peterx@redhat.com>
> >
> > Unfortunately with virtio it's a regression, as it used to
> > work with iommu. So I'm afraid we need to look into supporting
> > arbitrary order right now :(
> >
> 
> Hi,
> 
> A fast way to do it is to initialize iommu with a new keyword, like
>      -iommu intel-iommu
> Or maybe use the existing "-object" somehow ?
>  From vl.c comments:
>    "Initial object creation happens before all other
>     QEMU data types are created.... "
> 
> Another idea is to add a third run on QEMU cmd line arguments,
> but this would affect the whole system.
> 
> However having an ordering system beats all other ideas.

The ordering should be explicit in the command line.  Since we do not have
cycles, it should be possible.

The best solution would have been to add something like

   -device intel-iommu,id=root-complex-iommu -global mch.iommu=root-complex-iommu

but it's too late for that.

Paolo

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Peter Xu]

On Wed, Mar 08, 2017 at 03:35:50AM -0500, Paolo Bonzini wrote:

[...]

> The ordering should be explicit in the command line.  Since we do not have
> cycles, it should be possible.

Current QEMU should have no restriction on parameter ordering, right?
Or do we have any existing case that:

  "$QEMU -parameter1 -parameter2"

will work, while...

  "$QEMU -parameter2 -parameter1"

won't work?

Thanks,

> 
> The best solution would have been to add something like
> 
>    -device intel-iommu,id=root-complex-iommu -global mch.iommu=root-complex-iommu
> 
> but it's too late for that.

-- peterx

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Gerd Hoffmann]

> Current QEMU should have no restriction on parameter ordering, right?
> Or do we have any existing case that:
> 
>   "$QEMU -parameter1 -parameter2"
> 
> will work, while...
> 
>   "$QEMU -parameter2 -parameter1"
> 
> won't work?

Well, we have.  If you, for example, add a scsi disk, you have to add a
scsi host adapter first.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Peter Xu]

On Wed, Mar 08, 2017 at 10:25:41AM +0100, Gerd Hoffmann wrote:
> > Current QEMU should have no restriction on parameter ordering, right?
> > Or do we have any existing case that:
> > 
> >   "$QEMU -parameter1 -parameter2"
> > 
> > will work, while...
> > 
> >   "$QEMU -parameter2 -parameter1"
> > 
> > won't work?
> 
> Well, we have.  If you, for example, add a scsi disk, you have to add a
> scsi host adapter first.

I wasn't aware of that... Thanks!

-- peterx

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Jason Wang]

On 2017年03月08日 16:35, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Marcel Apfelbaum" <marcel@redhat.com>
>> To: "Michael S. Tsirkin" <mst@redhat.com>, "Peter Xu" <peterx@redhat.com>
>> Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>, "yi l liu" <yi.l.liu@intel.com>, "Jintack Lim"
>> <jintack@cs.columbia.edu>, "Jason Wang" <jasowang@redhat.com>, "Alex Williamson" <alex.williamson@redhat.com>
>> Sent: Thursday, March 2, 2017 7:24:44 AM
>> Subject: Re: [PATCH v3] intel_iommu: check misordered init when realize
>>
>> On 03/02/2017 07:13 AM, Michael S. Tsirkin wrote:
>>> On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
>>>> Intel vIOMMU devices are created with "-device" parameter, while here
>>>> actually we need to make sure the dmar device be created before other
>>>> PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
>>>> will be setup correctly before realizations of those PCI devices (it is
>>>> legal that PCI device fetch these info during its realization). Now this
>>>> ordering yet cannot be achieved elsewhere, and devices will be created
>>>> in the order that user specified. We need to avoid that.
>>>>
>>>> This patch tries to detect this kind of misordering issue during init of
>>>> VT-d device, then report to guest if misordering happened. In the
>>>> future, we can provide something better to solve it, e.g., to support
>>>> device init ordering, then we can live without this patch.
>>>>
>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> Unfortunately with virtio it's a regression, as it used to
>>> work with iommu. So I'm afraid we need to look into supporting
>>> arbitrary order right now :(
>>>
>> Hi,
>>
>> A fast way to do it is to initialize iommu with a new keyword, like
>>       -iommu intel-iommu
>> Or maybe use the existing "-object" somehow ?
>>   From vl.c comments:
>>     "Initial object creation happens before all other
>>      QEMU data types are created.... "
>>
>> Another idea is to add a third run on QEMU cmd line arguments,
>> but this would affect the whole system.
>>
>> However having an ordering system beats all other ideas.
> The ordering should be explicit in the command line.  Since we do not have
> cycles, it should be possible.
>
> The best solution would have been to add something like
>
>     -device intel-iommu,id=root-complex-iommu -global mch.iommu=root-complex-iommu
>
> but it's too late for that.
>
> Paolo
>

Yes, it's probably too late other than having workarounds for 2.9. And 
to eliminate similar issues, we may initializing the pci devices by 
traversing the PCI tree from root complex in level order.

Thanks

Re: [Qemu-devel] [PATCH v3] intel_iommu: check misordered init when realize

[Marcel Apfelbaum]

On 03/08/2017 12:08 PM, Jason Wang wrote:
>
>
> On 2017年03月08日 16:35, Paolo Bonzini wrote:
>>
>> ----- Original Message -----
>>> From: "Marcel Apfelbaum" <marcel@redhat.com>
>>> To: "Michael S. Tsirkin" <mst@redhat.com>, "Peter Xu" <peterx@redhat.com>
>>> Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>, "yi l liu" <yi.l.liu@intel.com>, "Jintack Lim"
>>> <jintack@cs.columbia.edu>, "Jason Wang" <jasowang@redhat.com>, "Alex Williamson" <alex.williamson@redhat.com>
>>> Sent: Thursday, March 2, 2017 7:24:44 AM
>>> Subject: Re: [PATCH v3] intel_iommu: check misordered init when realize
>>>
>>> On 03/02/2017 07:13 AM, Michael S. Tsirkin wrote:
>>>> On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
>>>>> Intel vIOMMU devices are created with "-device" parameter, while here
>>>>> actually we need to make sure the dmar device be created before other
>>>>> PCI devices (like vfio-pci, virtio-pci ones) so that we know iommu_fn
>>>>> will be setup correctly before realizations of those PCI devices (it is
>>>>> legal that PCI device fetch these info during its realization). Now this
>>>>> ordering yet cannot be achieved elsewhere, and devices will be created
>>>>> in the order that user specified. We need to avoid that.
>>>>>
>>>>> This patch tries to detect this kind of misordering issue during init of
>>>>> VT-d device, then report to guest if misordering happened. In the
>>>>> future, we can provide something better to solve it, e.g., to support
>>>>> device init ordering, then we can live without this patch.
>>>>>
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>> Unfortunately with virtio it's a regression, as it used to
>>>> work with iommu. So I'm afraid we need to look into supporting
>>>> arbitrary order right now :(
>>>>
>>> Hi,
>>>
>>> A fast way to do it is to initialize iommu with a new keyword, like
>>>       -iommu intel-iommu
>>> Or maybe use the existing "-object" somehow ?
>>>   From vl.c comments:
>>>     "Initial object creation happens before all other
>>>      QEMU data types are created.... "
>>>
>>> Another idea is to add a third run on QEMU cmd line arguments,
>>> but this would affect the whole system.
>>>
>>> However having an ordering system beats all other ideas.
>> The ordering should be explicit in the command line.  Since we do not have
>> cycles, it should be possible.
>>
>> The best solution would have been to add something like
>>
>>     -device intel-iommu,id=root-complex-iommu -global mch.iommu=root-complex-iommu
>>
>> but it's too late for that.
>>
>> Paolo
>>
>
> Yes, it's probably too late other than having workarounds for 2.9.

I suggested to Laine to require libvirt to initialize first the iommu device.
and since most QEMU users use libvirt anyway, we may just be OK (kind of).

> And to eliminate similar issues, we may initializing the pci devices by traversing the PCI tree from root complex in level order.

Interesting idea, we'll explore it for 2.10.
But we need initialization order for all machine components, not only PCI.

Thanks,
Marcel

>
> Thanks