[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v2 00/26] arm64: Dom0 ITS emulation



Hi Andre,

   I see following issues when running on ThunderX platform with your patches.
I have debugged and patched/workaround few issues. For issue (5) I
need your  inputs.

1) Your code base fails to boot xen. Fails at dom0 memory allocation.
    To overcome this I have rebased your patches on top of 4.8 stable
release and this issue is not seen.

2)  ITS is not initialized if GICv2 info is not found in GICv3 dt
node. But having GICv2 info is not
   mandatory. So in the below code if GICv2 info is not found ITS is
not initialized.

static void __init gicv3_dt_init (void)
{
   ...
    /*
     * For GICv3 supporting GICv2, GICC and GICV base address will be
     * provided.
     */
    res = dt_device_get_address(node, 1 + gicv3.rdist_count,
                                &cbase, &csize);

-    if ( res )    // <==  This check needs to be managed properly?
-        return;

    dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
                          &vbase, &vsize);

    /* Check for ITS child nodes and build the host ITS list accordingly. */
    gicv3_its_dt_init(node);

}

3) Dom0 is not calling PHYSDEVOP_manage_pci_add for all the pci devices
   instead it is making PHYSDEVOP_pci_device_add when segment exists.
   So I have made below changes so that ITS mapping is called for all
the devices.

4) Also put_domain() is throws panic. So I have dropped it in below code.

--- a/xen/arch/arm/physdev.c
+++ b/xen/arch/arm/physdev.c
@@ -17,27 +17,68 @@
 int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct physdev_manage_pci manage;
+    struct physdev_pci_device_add pci_add;
     struct domain *dom0;
     u32 devid;
     int ret;

     switch (cmd)
     {
         case PHYSDEVOP_manage_pci_add:
         case PHYSDEVOP_manage_pci_remove:
             if ( copy_from_guest(&manage, arg, 1) != 0 )
                 return -EFAULT;

             dom0 = hardware_domain;
             devid = manage.bus << 8 | manage.devfn;
             /* Allocate an ITS device table with space for 32 MSIs */
             ret = gicv3_its_map_device(dom0, devid, devid, 5,
                                        cmd == PHYSDEVOP_manage_pci_add);

-            put_domain(dom0);
             return ret;
+       case PHYSDEVOP_pci_device_add:
+            if ( copy_from_guest(&pci_add, arg, 1) != 0 )
+                return -EFAULT;
+            dom0 = hardware_domain;
+            devid = pci_add.seg << 16 | pci_add.bus << 8 | pci_add.devfn;
+
+            /* Allocate an ITS device table with space for 32 MSIs */
+            ret = gicv3_its_map_device(dom0, devid, devid, 5,
+                                       cmd == PHYSDEVOP_pci_device_add);
+
+            return ret;
     }

     gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd);
     return -ENOSYS;
 }


5)  With the above patch, Now I see ITS HW is throwing up Error 10801
where in it shows
devid is out of range in MAPD command.

See below log with some debug prints, where I have printed
GITS_IMP_CSER register for
every MAPD command.
(XEN) GITS_IMP_CSER 0x5e4000010801

This error is seen when seg is 1. This was not the case with previous
version of patches.
However GITS_TYPER from HW shows it support upto 20 bits.

pci 0000:01:10.1: [177d:a026] type 00 class 0x028000
pci 0000:01:10.1: BAR 0: [mem 0x87e0e1000000-0x87e0e13fffff 64bit]
(from Enhanced Allocation, properties 0x0)
pci 0000:01:10.1: BAR 4: [mem 0x87e0e1400000-0x87e0e17fffff 64bit]
(from Enhanced Allocation, properties 0x0)
(XEN) In do_physdev_op PHYSDEVOP_pci_device_add
(XEN) In do_physdev_op dev id 385 seg 0 bus 1 devfn 129
(XEN) In gicv3_its_map_device send MAPD for  dev id 385
(XEN) CMD[0] 0x18100000008 CMD[1] 0x4 CMD [2] 0x8000001ff6044200 CMD[3] 0x0
(XEN) GITS_IMP_TSER 0x0
(XEN) GITS_IMP_CSER 0x0
iommu: Adding device 0000:01:10.1 to group 41
pci 0000:01:00.0: disabling ASPM on pre-1.1 PCIe device.  You can
enable it with 'pcie_aspm=force'
pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
pci_bus 0000:02: busn_res: can not insert [bus 02-ff] under [bus
00-1f] (conflicts with (null) [bus 00-1f])
pci 0000:02:00.0: [177d:a01d] type 00 class 0x010400
pci 0000:02:00.0: BAR 0: [mem 0x870000000000-0x8700007fffff 64bit]
(from Enhanced Allocation, properties 0x0)
pci 0000:02:00.0: BAR 4: [mem 0x870000f00000-0x870000ffffff 64bit]
(from Enhanced Allocation, properties 0x0)
(XEN) In do_physdev_op PHYSDEVOP_pci_device_add
(XEN) In do_physdev_op dev id 512 seg 0 bus 2 devfn 0
(XEN) In gicv3_its_map_device send MAPD for  dev id 512
(XEN) CMD[0] 0x20000000008 CMD[1] 0x4 CMD [2] 0x8000001ff6044400 CMD[3] 0x0
(XEN) GITS_IMP_TSER 0x0
(XEN) GITS_IMP_CSER 0x0
iommu: Adding device 0000:02:00.0 to group 42
pci 0000:02:00.0: disabling ASPM on pre-1.1 PCIe device.  You can
enable it with 'pcie_aspm=force'
pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
pci_bus 0000:03: busn_res: can not insert [bus 03-ff] under [bus
00-1f] (conflicts with (null) [bus 00-1f])
pci 0000:03:00.0: [177d:a01a] type 00 class 0x120000
pci 0000:03:00.0: BAR 0: [mem 0x838000000000-0x8380003fffff 64bit]
(from Enhanced Allocation, properties 0x0)
pci 0000:03:00.0: BAR 4: [mem 0x838000f00000-0x838000ffffff 64bit]
(from Enhanced Allocation, properties 0x0)
(XEN) In do_physdev_op PHYSDEVOP_pci_device_add
(XEN) In do_physdev_op dev id 768 seg 0 bus 3 devfn 0
(XEN) In gicv3_its_map_device send MAPD for  dev id 768
(XEN) CMD[0] 0x30000000008 CMD[1] 0x4 CMD [2] 0x8000001ff6044500 CMD[3] 0x0
(XEN) GITS_IMP_TSER 0x0
(XEN) GITS_IMP_CSER 0x0
iommu: Adding device 0000:03:00.0 to group 43
pci 0000:03:00.0: disabling ASPM on pre-1.1 PCIe device.  You can
enable it with 'pcie_aspm=force'
pci_bus 0000:03: busn_res: [bus 03-ff] end is updated to 03
pci_bus 0000:04: busn_res: can not insert [bus 04-ff] under [bus
00-1f] (conflicts with (null) [bus 00-1f])
pci 0000:04:00.0: [177d:a019] type 00 class 0x120000
pci 0000:04:00.0: BAR 0: [mem 0x846000000000-0x8467ffffffff 64bit]
(from Enhanced Allocation, properties 0x0)
pci 0000:04:00.0: BAR 4: [mem 0x846a00000000-0x846a000fffff 64bit]
(from Enhanced Allocation, properties 0x0)
(XEN) In do_physdev_op PHYSDEVOP_pci_device_add
(XEN) In do_physdev_op dev id 1024 seg 0 bus 4 devfn 0
(XEN) In gicv3_its_map_device send MAPD for  dev id 1024
(XEN) CMD[0] 0x40000000008 CMD[1] 0x4 CMD [2] 0x8000001ff6044700 CMD[3] 0x0
(XEN) GITS_IMP_TSER 0x0
(XEN) GITS_IMP_CSER 0x0
iommu: Adding device 0000:04:00.0 to group 44
pci 0000:04:00.0: disabling ASPM on pre-1.1 PCIe device.  You can
enable it with 'pcie_aspm=force'
pci_bus 0000:04: busn_res: [bus 04-ff] end is updated to 04
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:14.0: PCI bridge to [bus 02]
pci 0000:00:15.0: PCI bridge to [bus 03]
pci 0000:00:16.0: PCI bridge to [bus 04]
OF: PCI: host bridge /soc@0/pci@849000000000 ranges:
OF: PCI:   MEM 0x810000000000..0x817fffffffff -> 0x810000000000
pci-host-generic 849000000000.pci: ECAM at [mem
0x849000000000-0x849001ffffff] for [bus 00-1f]
pci-host-generic 849000000000.pci: PCI host bridge to bus 0001:00
pci_bus 0001:00: root bus resource [bus 00-1f]
pci_bus 0001:00: root bus resource [mem 0x810000000000-0x817fffffffff]
pci 0001:00:08.0: [177d:a01c] type 00 class 0x010601
pci 0001:00:08.0: BAR 0: [mem 0x814000000000-0x8140001fffff 64bit]
(from Enhanced Allocation, properties 0x0)
pci 0001:00:08.0: BAR 4: [mem 0x814000200000-0x8140002fffff 64bit]
(from Enhanced Allocation, properties 0x0)
(XEN) In do_physdev_op PHYSDEVOP_pci_device_add
(XEN) In do_physdev_op dev id 65600 seg 1 bus 0 devfn 64
(XEN) In gicv3_its_map_device send MAPD for  dev id 65600
(XEN) CMD[0] 0x1004000000008 CMD[1] 0x4 CMD [2] 0x8000001ff6044900 CMD[3] 0x0
(XEN) GITS_IMP_TSER 0x0
(XEN) GITS_IMP_CSER 0x5e4000010801  // <== MAPD deviceid Out of Range error.

6) I have also seen panic in gicv3_assign_guest_event() when called
from dom0 DISCARD ITS command.
   Not debugged much.

Regards
Vijay

On Thu, Dec 22, 2016 at 11:54 PM, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Hi,
>
> this is a reworked version of the Dom0 GICv3-ITS emulation series.
> This is still not fully where I want it and has some loose bits and
> pieces still, but since there are significant changes in the architecture
> I wanted to have an opinion before going ahead and replacing every single
> number with a named constant ;-) If that smells like a "send out before
> the end of the year", you are spot on.
>
> This series introduces ARM GICv3 ITS emulation, for now restricted to
> Dom0 only. The ITS is an interrupt controller widget providing a
> sophisticated way to deal with MSIs in a scalable manner.
> For hardware which relies on the ITS to provide interrupts for its
> peripherals this code is needed to get a machine booted into Dom0 at all.
> ITS emulation for DomUs is only really useful with PCI passthrough,
> which is not yet available for ARM. It is expected that this feature
> will be co-developed with the ITS DomU code. However this code drop here
> considered DomU emulation already, to keep later architectural changes
> to a minimum.
>
> Some generic design principles:
>
> * The current GIC code statically allocates structures for each supported
> IRQ (both for the host and the guest), which due to the potentially
> millions of LPI interrupts is not feasible to copy for the ITS.
> So we refrain from introducing the ITS as a first class Xen interrupt
> controller, also we don't hold struct irq_desc's or struct pending_irq's
> for each possible LPI.
> Fortunately LPIs are only interesting to guests, so we get away with
> storing only the virtual IRQ number and the guest VCPU for each allocated
> host LPI, which can be stashed into one uint64_t. This data is stored in
> a two-level table, which is both memory efficient and quick to access.
> We hook into the existing IRQ handling and VGIC code to avoid accessing
> the normal structures, providing alternative methods for getting the
> needed information (priority, is enabled?) for LPIs.
> For interrupts which are queued to or are actually in a guest we
> allocate struct pending_irq's on demand. As it is expected that only a
> very small number of interrupts is ever on a VCPU at the same time, this
> seems like the best approach. For now allocated structs are re-used and
> held in a linked list.
>
> * On the guest side we (later will) have to deal with malicious guests
> trying to hog Xen with mapping requests for a lot of LPIs, for instance.
> As the ITS actually uses system memory for storing status information,
> we use this memory (which the guest has to provide) to naturally limit
> a guest. For those tables which are page sized (devices, collections (CPUs),
> LPI properties) we map those pages into Xen, so we can easily access
> them from the virtual GIC code.
> Unfortunately the actual interrupt mapping tables are not necessarily
> page aligned, also can be much smaller than a page, so mapping all of
> them permanently is fiddly. As ITS commands in need to iterate those
> tables are pretty rare after all, we for now map them on demand upon
> emulating a virtual ITS command.
>
> * An obvious approach to handling some guest ITS commands would be to
> propagate them to the host, for instance to map devices and LPIs and
> to enable or disable LPIs.
> However this (later with DomU support) will create an attack vector, as
> a malicious guest could try to fill the host command queue with
> propagated commands.
> So in contrast to the previous RFC post this version now completely avoids
> this situation. For mapping devices and LPIs we rely on this being done
> via a hypercall prior to the actual guest run. For enabling and disabling
> LPIs we keep this bit on the virtual side and let LPIs always be enabled
> on the host side, dealing with the consequences this approach creates.
>
> This series is still a draft, with some known and many unknown issues.
> I made ITS support a Kconfig option, also it is only supported on arm64.
> This leads to some hideous constructs like an #ifdef'ed header file with
> empty function stubs, but I guess we can clean this up later in the
> upstreaming process.
>
> There are numerous changes compared to the last post, mainly affecting
> the now missing ITS command progagation. I also added locking to the
> "usual suspects" data structures.
> I picked some low hanging fruits from the review comments.
> Things I haven't addresses well is the whole memory management, in terms
> of marking pages r/o for a guest or allocating Xen memory from the proper
> bucket. This will be addresses with the next post.
>
> For now this code happens to boot Dom0 on an ARM fast model with ITS
> support. I still haven't had the chance to get hold of a Xen supported
> hardware platform with an ITS yet, so running on real hardware is a bit
> terra incognita.
>
> The code can also be found on the its/rfc-v2 branch here:
> git://linux-arm.org/xen-ap.git
> http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/its/rfc-v2
>
> Cheers,
> Andre
>
> Andre Przywara (26):
>   ARM: GICv3 ITS: parse and store ITS subnodes from hardware DT
>   ARM: GICv3: allocate LPI pending and property table
>   ARM: GICv3 ITS: allocate device and collection table
>   ARM: GICv3 ITS: map ITS command buffer
>   ARM: GICv3 ITS: introduce ITS command handling
>   ARM: GICv3 ITS: introduce device mapping
>   ARM: GICv3 ITS: introduce host LPI array
>   ARM: GICv3 ITS: map device and LPIs to the ITS on physdev_op hypercall
>   ARM: GICv3: introduce separate pending_irq structs for LPIs
>   ARM: GICv3: forward pending LPIs to guests
>   ARM: GICv3: enable ITS and LPIs on the host
>   ARM: vGICv3: handle virtual LPI pending and property tables
>   ARM: vGICv3: Handle disabled LPIs
>   ARM: vGICv3: introduce basic ITS emulation bits
>   ARM: vITS: handle CLEAR command
>   ARM: vITS: handle INT command
>   ARM: vITS: handle MAPC command
>   ARM: vITS: handle MAPD command
>   ARM: vITS: handle MAPTI command
>   ARM: vITS: handle MOVI command
>   ARM: vITS: handle DISCARD command
>   ARM: vITS: handle INV command
>   ARM: vITS: handle INVALL command
>   ARM: vITS: create and initialize virtual ITSes for Dom0
>   ARM: vITS: create ITS subnodes for Dom0 DT
>   ARM: vGIC: advertising LPI support
>
>  xen/arch/arm/Kconfig              |  20 +
>  xen/arch/arm/Makefile             |   2 +
>  xen/arch/arm/efi/efi-boot.h       |   1 -
>  xen/arch/arm/gic-its.c            | 934 
> ++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c             |  92 +++-
>  xen/arch/arm/gic.c                |   9 +-
>  xen/arch/arm/physdev.c            |  24 +
>  xen/arch/arm/vgic-its.c           | 842 ++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v3.c            | 274 +++++++++--
>  xen/arch/arm/vgic.c               |  71 ++-
>  xen/include/asm-arm/bitops.h      |   2 +
>  xen/include/asm-arm/cache.h       |   4 +
>  xen/include/asm-arm/domain.h      |  13 +-
>  xen/include/asm-arm/gic-its.h     | 217 +++++++++
>  xen/include/asm-arm/gic_v3_defs.h |  67 ++-
>  xen/include/asm-arm/irq.h         |   8 +
>  xen/include/asm-arm/vgic.h        |  15 +
>  17 files changed, 2558 insertions(+), 37 deletions(-)
>  create mode 100644 xen/arch/arm/gic-its.c
>  create mode 100644 xen/arch/arm/vgic-its.c
>  create mode 100644 xen/include/asm-arm/gic-its.h
>
> --
> 2.9.0
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.