This patchset is to add Xen vIOMMU device model and handle
irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
and the new device module in Qemu works as hypercall wrappers to
create and destroy vIOMMU in hypervisor.

Xen only supports emulated I440 and so we enable vIOMMU with emulated
I440 chipset. This works on Linux and Windows guest.


Chao Gao (4):
  I440: Allow adding sysbus devices with -device on I440
  Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
  xen-pt: bind/unbind interrupt remapping format MSI
  msi: taking interrupt format into consideration during judging a pirq
    is binded with a event channel

 hw/i386/pc_piix.c             |   1 +
 hw/pci/msi.c                  |   5 +-
 hw/pci/msix.c                 |   4 +-
 hw/xen/Makefile.objs          |   1 +
 hw/xen/xen_pt_msi.c           |  38 ++++++++++----
 hw/xen/xen_viommu.c           | 116 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic-msidef.h |   1 +
 include/hw/xen/xen.h          |   2 +-
 xen-hvm-stub.c                |   2 +-
 xen-hvm.c                     |   7 ++-
 10 files changed, 162 insertions(+), 15 deletions(-)
 create mode 100644 hw/xen/xen_viommu.c

-- 
1.8.3.1

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1489750157-17401-1-git-send-email-tianyu.lan@intel.com
Subject: [Qemu-devel] [RFC PATCH 0/4] Qemu: Add Xen vIOMMU support
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1489750157-17401-1-git-send-email-tianyu.lan@intel.com -> patchew/1489750157-17401-1-git-send-email-tianyu.lan@intel.com
Switched to a new branch 'test'
0f16022 msi: taking interrupt format into consideration during judging a pirq is binded with a event channel
62b6353 xen-pt: bind/unbind interrupt remapping format MSI
0900f2b Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
7cb3e06 I440: Allow adding sysbus devices with -device on I440

=== OUTPUT BEGIN ===
Checking PATCH 1/4: I440: Allow adding sysbus devices with -device on I440...
Checking PATCH 2/4: Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen...
ERROR: trailing whitespace
#75: FILE: hw/xen/xen_viommu.c:48:
+    $

ERROR: trailing whitespace
#79: FILE: hw/xen/xen_viommu.c:52:
+    rc = xenstore_read_uint64(viommu_path, "base_addr", &s->base_addr);  $

ERROR: trailing whitespace
#116: FILE: hw/xen/xen_viommu.c:89:
+        rc = xc_viommu_destroy(xen_xc, xen_domid, s->id); $

ERROR: trailing whitespace
#140: FILE: hw/xen/xen_viommu.c:113:
+    type_register_static(&xen_viommu_info); $

total: 4 errors, 0 warnings, 120 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: xen-pt: bind/unbind interrupt remapping format MSI...
ERROR: else should follow close brace '}'
#36: FILE: hw/xen/xen_pt_msi.c:179:
+    }
+    else {

ERROR: space prohibited after that open parenthesis '('
#54: FILE: hw/xen/xen_pt_msi.c:214:
+        if ( addr & MSI_ADDR_IM_MASK ) {

ERROR: space prohibited before that close parenthesis ')'
#54: FILE: hw/xen/xen_pt_msi.c:214:
+        if ( addr & MSI_ADDR_IM_MASK ) {

WARNING: line over 80 characters
#55: FILE: hw/xen/xen_pt_msi.c:215:
+            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",

ERROR: line over 90 characters
#60: FILE: hw/xen/xen_pt_msi.c:220:
+                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",

WARNING: line over 80 characters
#68: FILE: hw/xen/xen_pt_msi.c:228:
+            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);

ERROR: line over 90 characters
#70: FILE: hw/xen/xen_pt_msi.c:230:
+                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",

total: 5 errors, 2 warnings, 61 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: msi: taking interrupt format into consideration during judging a pirq is binded with a event channel...
ERROR: space prohibited after that open parenthesis '('
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )

ERROR: space prohibited before that close parenthesis ')'
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )

ERROR: braces {} are necessary for all arms of this statement
#111: FILE: xen-hvm.c:154:
+    if ( msi_addr_lo & 0x10 )
[...]

total: 3 errors, 0 warnings, 67 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

On 17/03/2017 12:29, Lan Tianyu wrote:
> This patchset is to add Xen vIOMMU device model and handle
> irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
> and the new device module in Qemu works as hypercall wrappers to
> create and destroy vIOMMU in hypervisor.
> 
> Xen only supports emulated I440 and so we enable vIOMMU with emulated
> I440 chipset. This works on Linux and Windows guest.

Any plans to change this?  Why is Xen not able to use Q35 with Intel
IOMMU, with only special hooks for interrupt remapping?

Paolo

> Chao Gao (4):
>   I440: Allow adding sysbus devices with -device on I440
>   Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
>   xen-pt: bind/unbind interrupt remapping format MSI
>   msi: taking interrupt format into consideration during judging a pirq
>     is binded with a event channel

On Fri, 17 Mar 2017, Paolo Bonzini wrote:
> On 17/03/2017 12:29, Lan Tianyu wrote:
> > This patchset is to add Xen vIOMMU device model and handle
> > irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
> > and the new device module in Qemu works as hypercall wrappers to
> > create and destroy vIOMMU in hypervisor.
> > 
> > Xen only supports emulated I440 and so we enable vIOMMU with emulated
> > I440 chipset. This works on Linux and Windows guest.
> 
> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> IOMMU, with only special hooks for interrupt remapping?

CC'ing Anthony that worked on it in the past


> > Chao Gao (4):
> >   I440: Allow adding sysbus devices with -device on I440
> >   Xen: add a dummy vIOMMU to create/destroy vIOMMU in Xen
> >   xen-pt: bind/unbind interrupt remapping format MSI
> >   msi: taking interrupt format into consideration during judging a pirq
> >     is binded with a event channel
> 

On 2017年03月17日 22:48, Paolo Bonzini wrote:
> 
> 
> On 17/03/2017 12:29, Lan Tianyu wrote:
>> This patchset is to add Xen vIOMMU device model and handle
>> irq remapping stuffs. Xen vIOMMU emulation is in the Xen hypervisor
>> and the new device module in Qemu works as hypercall wrappers to
>> create and destroy vIOMMU in hypervisor.
>>
>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
>> I440 chipset. This works on Linux and Windows guest.
> 
> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> IOMMU, with only special hooks for interrupt remapping?
> 
> Paolo
> 

Hi Paolo:
Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
in Qemu and create counterpart in Xen hypervisor. The reasons are
 1) Avoid round trips between Qemu and Xen hypervisor
 2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
vMSI and so on).

We don't have plan to enable Q35 for Xen now. There is no dependency
between Q35 emulation and vIOMMU implementation from our test. As
Stefano mentioned, Anthony worked on the Q35 support before. These two
tasks can be done in parallel.

-- 
Best regards
Tianyu Lan

On 20/03/2017 03:40, Lan Tianyu wrote:
>>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
>>> I440 chipset. This works on Linux and Windows guest.
>> Any plans to change this?  Why is Xen not able to use Q35 with Intel
>> IOMMU, with only special hooks for interrupt remapping?
>>
>> Paolo
>>
> Hi Paolo:
> Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
> in Qemu and create counterpart in Xen hypervisor. The reasons are
>  1) Avoid round trips between Qemu and Xen hypervisor
>  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
> vMSI and so on).

Fair enough, though I'd be worried about increasing the attack surface
of the hypervisor.  For KVM, for example, IOMMU emulation requires using
the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
and back to QEMU.

Also, I think this series is missing changes to support IOMMU
translation in the vIOMMU device model.

Paolo

> We don't have plan to enable Q35 for Xen now. There is no dependency
> between Q35 emulation and vIOMMU implementation from our test. As
> Stefano mentioned, Anthony worked on the Q35 support before. These two
> tasks can be done in parallel.

On Mon, Mar 20, 2017 at 12:38:41PM +0100, Paolo Bonzini wrote:
> 
> 
> On 20/03/2017 03:40, Lan Tianyu wrote:
> >>> Xen only supports emulated I440 and so we enable vIOMMU with emulated
> >>> I440 chipset. This works on Linux and Windows guest.
> >> Any plans to change this?  Why is Xen not able to use Q35 with Intel
> >> IOMMU, with only special hooks for interrupt remapping?
> >>
> >> Paolo
> >>
> > Hi Paolo:
> > Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
> > in Qemu and create counterpart in Xen hypervisor. The reasons are
> >  1) Avoid round trips between Qemu and Xen hypervisor
> >  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
> > vMSI and so on).
> 
> Fair enough, though I'd be worried about increasing the attack surface
> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
> and back to QEMU.

Yes, that's right, we are increasing the surface of attack. But Xen also needs
it in order to support APIC IDs > 255 on PVH guests (that have a local APIC but
no QEMU).

Roger.

On 20/03/2017 15:17, Roger Pau Monné wrote:
>>> Hi Paolo:
>>> Thanks for review. For Xen side, we won't reuse Intel IOMMU device model
>>> in Qemu and create counterpart in Xen hypervisor. The reasons are
>>>  1) Avoid round trips between Qemu and Xen hypervisor
>>>  2) Ease of integration with the rest part of the hypervisor(vIOAPIC,
>>> vMSI and so on).
>> Fair enough, though I'd be worried about increasing the attack surface
>> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
>> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
>> and back to QEMU.
> Yes, that's right, we are increasing the surface of attack. But Xen also needs
> it in order to support APIC IDs > 255 on PVH guests (that have a local APIC but
> no QEMU).

Not necessarily, you only need x2APIC support for that in the local APIC
emulation.  The MSI hypercalls (similar to KVM's MSI ioctls) can be
extended to accept x2APIC VCPU ids.

Paolo

On 2017年03月20日 19:38, Paolo Bonzini wrote:
> Fair enough, though I'd be worried about increasing the attack surface
> of the hypervisor.  For KVM, for example, IOMMU emulation requires using
> the "split irqchip" feature to move the PIC and IOAPIC out of the kernel
> and back to QEMU.

Yes, just like Roger mentioned we also need to support no-qemu mode on
Xen and this is tradeoff result.

> 
> Also, I think this series is missing changes to support IOMMU
> translation in the vIOMMU device model.

Yes, this series just enabled vIOMMU's irq remapping function and we
need to pass virtual device's DMA request to Xen hypervisor for
translation when enable DMA translation.

-- 
Best regards
Tianyu Lan