[Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Michael S. Tsirkin]

(((sid) >> 8) && 0xff)  makes no sense
(((sid) >> 8) & 0xff) seems to be what was meant.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Compile-tested only.

 include/hw/i386/intel_iommu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index f4701e1..e321ee4 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -37,7 +37,7 @@
 #define VTD_PCI_DEVFN_MAX           256
 #define VTD_PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define VTD_PCI_FUNC(devfn)         ((devfn) & 0x07)
-#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) && 0xff)
+#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) & 0xff)
 #define VTD_SID_TO_DEVFN(sid)       ((sid) & 0xff)
 
 #define DMAR_REG_SIZE               0x230
-- 
MST

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> (((sid) >> 8) && 0xff)  makes no sense
> (((sid) >> 8) & 0xff) seems to be what was meant.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>

Actually by the reporter of https://bugs.launchpad.net/bugs/1382477

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Compile-tested only.
>
>  include/hw/i386/intel_iommu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index f4701e1..e321ee4 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -37,7 +37,7 @@
>  #define VTD_PCI_DEVFN_MAX           256
>  #define VTD_PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define VTD_PCI_FUNC(devfn)         ((devfn) & 0x07)
> -#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) && 0xff)
> +#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) & 0xff)
>  #define VTD_SID_TO_DEVFN(sid)       ((sid) & 0xff)
>  
>  #define DMAR_REG_SIZE               0x230

Can't find the spec right now, but it looks plausible enough.

Only use is in vtd_context_device_invalidate().  Bug's impact isn't
obvious to me.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Le Tan]

Hi Markus,

2014-10-20 19:41 GMT+08:00 Markus Armbruster <armbru@redhat.com>:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> (((sid) >> 8) && 0xff)  makes no sense
>> (((sid) >> 8) & 0xff) seems to be what was meant.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>
> Actually by the reporter of https://bugs.launchpad.net/bugs/1382477
>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>
>> Compile-tested only.
>>
>>  include/hw/i386/intel_iommu.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>> index f4701e1..e321ee4 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -37,7 +37,7 @@
>>  #define VTD_PCI_DEVFN_MAX           256
>>  #define VTD_PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>>  #define VTD_PCI_FUNC(devfn)         ((devfn) & 0x07)
>> -#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) && 0xff)
>> +#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) & 0xff)
>>  #define VTD_SID_TO_DEVFN(sid)       ((sid) & 0xff)
>>
>>  #define DMAR_REG_SIZE               0x230
>
> Can't find the spec right now, but it looks plausible enough.

Yes, this is a typo. I am sorry that I introduced such a mistake.
The spec is here in Section 3.4 :
http://www.intel.com/content/www/us/en/intelligent-systems/intel-technology/vt-directed-io-spec.html

VTD_SID_TO_BUS(sid) is intended to be used to get the bus id from the
source identifier.

Thanks very much!

Regards,
Le

> Only use is in vtd_context_device_invalidate().  Bug's impact isn't
> obvious to me.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Michael S. Tsirkin]

WRT intel_iommu, it does not yet seem to be as fully functional as I
hoped.  People also discussed the best way to handle virtio versus iommu
(it bypasses it ATM).
I'd like to suggest we hide the iommu from the command line
help for 2.2, this way people don't activate it mistakenly.

Let's make it more complete and then enable for 2.3.

Thoughts?

-- 
MST

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Jan Kiszka]

On 2014-10-20 16:23, Michael S. Tsirkin wrote:
> 
> WRT intel_iommu, it does not yet seem to be as fully functional as I
> hoped.

What is missing or broken?

>  People also discussed the best way to handle virtio versus iommu
> (it bypasses it ATM).

We need to declare in the ACPI tables that the emulated IOMMUs do not
cover those "special" virtio devices.

> I'd like to suggest we hide the iommu from the command line
> help for 2.2, this way people don't activate it mistakenly.
> 
> Let's make it more complete and then enable for 2.3.
> 
> Thoughts?

We need the list of issues on the table, then we can decide.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Michael S. Tsirkin]

On Mon, Oct 20, 2014 at 04:37:15PM +0200, Jan Kiszka wrote:
> On 2014-10-20 16:23, Michael S. Tsirkin wrote:
> > 
> > WRT intel_iommu, it does not yet seem to be as fully functional as I
> > hoped.
> 
> What is missing or broken?

PCI to PCI bridges don't work, do they?

> >  People also discussed the best way to handle virtio versus iommu
> > (it bypasses it ATM).
> 
> We need to declare in the ACPI tables that the emulated IOMMUs do not
> cover those "special" virtio devices.

I'd also be fine if we flat out disallow adding virtio
devices to such systems for now, or anything else that
does not work.
Patches for either approach aren't yet available, are they?

> > I'd like to suggest we hide the iommu from the command line
> > help for 2.2, this way people don't activate it mistakenly.
> > 
> > Let's make it more complete and then enable for 2.3.
> > 
> > Thoughts?
> 
> We need the list of issues on the table, then we can decide.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Jan Kiszka]

On 2014-10-20 19:20, Michael S. Tsirkin wrote:
> On Mon, Oct 20, 2014 at 04:37:15PM +0200, Jan Kiszka wrote:
>> On 2014-10-20 16:23, Michael S. Tsirkin wrote:
>>>
>>> WRT intel_iommu, it does not yet seem to be as fully functional as I
>>> hoped.
>>
>> What is missing or broken?
> 
> PCI to PCI bridges don't work, do they?

Knut had an answer, but there was something, right.

> 
>>>  People also discussed the best way to handle virtio versus iommu
>>> (it bypasses it ATM).
>>
>> We need to declare in the ACPI tables that the emulated IOMMUs do not
>> cover those "special" virtio devices.
> 
> I'd also be fine if we flat out disallow adding virtio
> devices to such systems for now, or anything else that
> does not work.

I wouldn't (heavily used in my test setups).

> Patches for either approach aren't yet available, are they?

If no one else is quicker, I'll look into the ACPI tables.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Knut Omang]

On Mon, 2014-10-20 at 17:23 +0300, Michael S. Tsirkin wrote:
> WRT intel_iommu, it does not yet seem to be as fully functional as I
> hoped.  People also discussed the best way to handle virtio versus iommu
> (it bypasses it ATM).
> I'd like to suggest we hide the iommu from the command line
> help for 2.2, this way people don't activate it mistakenly.
> 
> Let's make it more complete and then enable for 2.3.
> 
> Thoughts?

Note that you have to explicitly enable iommu support in the guest
(intel_iommu=on on the boot command line in the Linux case) for it to
have any effect apart from being visible in the DMAR table and logging
so it should not really do any harm.

>From my perspective the feature works well and I have been running a few
virtual machines with div.network workload stable using the additional 4
patches referred to in this post (Jan's two for interrupt remapping and
two bug fixes and enhancements from me for running behind bridges) :

https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02986.html

Latest version here:

https://github.com/knuto/qemu/tree/sriov_patches_v2

Give me a hint and I can rebase and post the two iommu patches, I
believe Jan wanted to do some more work on the interrupt remapping
first.

Knut

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On 2014-10-20 17:15, Knut Omang wrote:
> On Mon, 2014-10-20 at 17:23 +0300, Michael S. Tsirkin wrote:
>> WRT intel_iommu, it does not yet seem to be as fully functional as I
>> hoped.  People also discussed the best way to handle virtio versus iommu
>> (it bypasses it ATM).
>> I'd like to suggest we hide the iommu from the command line
>> help for 2.2, this way people don't activate it mistakenly.
>>
>> Let's make it more complete and then enable for 2.3.
>>
>> Thoughts?
> 
> Note that you have to explicitly enable iommu support in the guest
> (intel_iommu=on on the boot command line in the Linux case) for it to
> have any effect apart from being visible in the DMAR table and logging
> so it should not really do any harm.
> 
> From my perspective the feature works well and I have been running a few
> virtual machines with div.network workload stable using the additional 4
> patches referred to in this post (Jan's two for interrupt remapping and
> two bug fixes and enhancements from me for running behind bridges) :
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02986.html
> 
> Latest version here:
> 
> https://github.com/knuto/qemu/tree/sriov_patches_v2
> 
> Give me a hint and I can rebase and post the two iommu patches, I
> believe Jan wanted to do some more work on the interrupt remapping
> first.

You should avoid to depend on my series regarding upstreaming of fixes
or features that can be done independently. Did your bridging fixes
depend on IR? Can you break them up?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Knut Omang]

On Mon, 2014-10-20 at 20:18 +0200, Jan Kiszka wrote:
> On 2014-10-20 17:15, Knut Omang wrote:
> > On Mon, 2014-10-20 at 17:23 +0300, Michael S. Tsirkin wrote:
> >> WRT intel_iommu, it does not yet seem to be as fully functional as I
> >> hoped.  People also discussed the best way to handle virtio versus iommu
> >> (it bypasses it ATM).
> >> I'd like to suggest we hide the iommu from the command line
> >> help for 2.2, this way people don't activate it mistakenly.
> >>
> >> Let's make it more complete and then enable for 2.3.
> >>
> >> Thoughts?
> > 
> > Note that you have to explicitly enable iommu support in the guest
> > (intel_iommu=on on the boot command line in the Linux case) for it to
> > have any effect apart from being visible in the DMAR table and logging
> > so it should not really do any harm.
> > 
> > From my perspective the feature works well and I have been running a few
> > virtual machines with div.network workload stable using the additional 4
> > patches referred to in this post (Jan's two for interrupt remapping and
> > two bug fixes and enhancements from me for running behind bridges) :
> > 
> > https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02986.html
> > 
> > Latest version here:
> > 
> > https://github.com/knuto/qemu/tree/sriov_patches_v2
> > 
> > Give me a hint and I can rebase and post the two iommu patches, I
> > believe Jan wanted to do some more work on the interrupt remapping
> > first.
> 
> You should avoid to depend on my series regarding upstreaming of fixes
> or features that can be done independently. Did your bridging fixes
> depend on IR? Can you break them up?

Yes, that was my intention with the comment (to rebase to make my two
commits independent of your interrupt remapping commits) but I realize
the language in my comment was not clear - sorry for the confusion,..

Knut

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Knut Omang]

On Mon, 2014-10-20 at 21:03 +0200, Knut Omang wrote:
> On Mon, 2014-10-20 at 20:18 +0200, Jan Kiszka wrote:
> > On 2014-10-20 17:15, Knut Omang wrote:
> > > On Mon, 2014-10-20 at 17:23 +0300, Michael S. Tsirkin wrote:
> > >> WRT intel_iommu, it does not yet seem to be as fully functional as I
> > >> hoped.  People also discussed the best way to handle virtio versus iommu
> > >> (it bypasses it ATM).
> > >> I'd like to suggest we hide the iommu from the command line
> > >> help for 2.2, this way people don't activate it mistakenly.
> > >>
> > >> Let's make it more complete and then enable for 2.3.
> > >>
> > >> Thoughts?
> > > 
> > > Note that you have to explicitly enable iommu support in the guest
> > > (intel_iommu=on on the boot command line in the Linux case) for it to
> > > have any effect apart from being visible in the DMAR table and logging
> > > so it should not really do any harm.
> > > 
> > > From my perspective the feature works well and I have been running a few
> > > virtual machines with div.network workload stable using the additional 4
> > > patches referred to in this post (Jan's two for interrupt remapping and
> > > two bug fixes and enhancements from me for running behind bridges) :
> > > 
> > > https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02986.html
> > > 
> > > Latest version here:
> > > 
> > > https://github.com/knuto/qemu/tree/sriov_patches_v2

Sorry, I just realized this branch was for the wrong patch set -
rebasing now to repost just my two iommu fixes..

Knut

> > > Give me a hint and I can rebase and post the two iommu patches, I
> > > believe Jan wanted to do some more work on the interrupt remapping
> > > first.
>
> > You should avoid to depend on my series regarding upstreaming of fixes
> > or features that can be done independently. Did your bridging fixes
> > depend on IR? Can you break them up?
> 
> Yes, that was my intention with the comment (to rebase to make my two
> commits independent of your interrupt remapping commits) but I realize
> the language in my comment was not clear - sorry for the confusion,..
> 
> Knut
> 
> 

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Knut Omang]

On Mon, 2014-10-20 at 21:03 +0200, Knut Omang wrote:
> On Mon, 2014-10-20 at 20:18 +0200, Jan Kiszka wrote:
> > On 2014-10-20 17:15, Knut Omang wrote:
> > > On Mon, 2014-10-20 at 17:23 +0300, Michael S. Tsirkin wrote:
> > >> WRT intel_iommu, it does not yet seem to be as fully functional as I
> > >> hoped.  People also discussed the best way to handle virtio versus iommu
> > >> (it bypasses it ATM).
> > >> I'd like to suggest we hide the iommu from the command line
> > >> help for 2.2, this way people don't activate it mistakenly.
> > >>
> > >> Let's make it more complete and then enable for 2.3.
> > >>
> > >> Thoughts?
> > > 
> > > Note that you have to explicitly enable iommu support in the guest
> > > (intel_iommu=on on the boot command line in the Linux case) for it to
> > > have any effect apart from being visible in the DMAR table and logging
> > > so it should not really do any harm.
> > > 
> > > From my perspective the feature works well and I have been running a few
> > > virtual machines with div.network workload stable using the additional 4
> > > patches referred to in this post (Jan's two for interrupt remapping and
> > > two bug fixes and enhancements from me for running behind bridges) :
> > > 
> > > https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02986.html
> > > 
> > > Latest version here:
> > > 
> > > https://github.com/knuto/qemu/tree/sriov_patches_v2
> > > 
> > > Give me a hint and I can rebase and post the two iommu patches, I
> > > believe Jan wanted to do some more work on the interrupt remapping
> > > first.
> > 
> > You should avoid to depend on my series regarding upstreaming of fixes
> > or features that can be done independently. Did your bridging fixes
> > depend on IR? Can you break them up?
> 
> Yes, that was my intention with the comment (to rebase to make my two
> commits independent of your interrupt remapping commits) but I realize
> the language in my comment was not clear - sorry for the confusion,..

Jan,

https://github.com/knuto/qemu/tree/vtd_patches_v2

now contains my iommu fixes right on qemu master 
(which I just posted to the list)
with your interrupt remapping patches rebased on top of this.
This was a a bit of work after all so maybe you can save some time from
that.

Knut

Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Knut Omang]

On Mon, 2014-10-20 at 20:14 +0800, Le Tan wrote:
> Hi Markus,
> 
> 2014-10-20 19:41 GMT+08:00 Markus Armbruster <armbru@redhat.com>:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> >> (((sid) >> 8) && 0xff)  makes no sense
> >> (((sid) >> 8) & 0xff) seems to be what was meant.
> >>
> >> Suggested-by: Markus Armbruster <armbru@redhat.com>
> >
> > Actually by the reporter of https://bugs.launchpad.net/bugs/1382477
> >
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>
> >> Compile-tested only.
> >>
> >>  include/hw/i386/intel_iommu.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> >> index f4701e1..e321ee4 100644
> >> --- a/include/hw/i386/intel_iommu.h
> >> +++ b/include/hw/i386/intel_iommu.h
> >> @@ -37,7 +37,7 @@
> >>  #define VTD_PCI_DEVFN_MAX           256
> >>  #define VTD_PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
> >>  #define VTD_PCI_FUNC(devfn)         ((devfn) & 0x07)
> >> -#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) && 0xff)
> >> +#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) & 0xff)
> >>  #define VTD_SID_TO_DEVFN(sid)       ((sid) & 0xff)
> >>
> >>  #define DMAR_REG_SIZE               0x230
> >
> > Can't find the spec right now, but it looks plausible enough.
> 
> Yes, this is a typo. I am sorry that I introduced such a mistake.
> The spec is here in Section 3.4 :
> http://www.intel.com/content/www/us/en/intelligent-systems/intel-technology/vt-directed-io-spec.html
> 
> VTD_SID_TO_BUS(sid) is intended to be used to get the bus id from the
> source identifier.
> 
> Thanks very much!
> 
> Regards,
> Le
> 
> > Only use is in vtd_context_device_invalidate().  Bug's impact isn't
> > obvious to me.

It means that invalidation will not work as intended if a device is
place on another bus than 01:xx.x (or 0 in theory) as this bus_num
always evaluate to 1 or 0 as a boolean. I have been doing quite some
testing of the virtual iommu, but by luck or unluck depending on
viewpoint my device so far has always been in bus 1...

Note that input is always supposed to be a 16 bit value here so the and
is in theory not really necessary unless from a documentation and
precaution point of view.

Reviewed-by: Knut Omang <knut.omang@oracle.com>

Knut

> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
>