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

Re: [PATCH v7 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

Hi Bertrand,

On 15/10/2021 16:06, Bertrand Marquis wrote:
On 15 Oct 2021, at 15:30, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

On Fri, Oct 15, 2021 at 02:59:19PM +0100, Bertrand Marquis wrote:
From: Rahul Singh <rahul.singh@xxxxxxx>

The existing VPCI support available for X86 is adapted for Arm.
When the device is added to XEN via the hyper call
“PHYSDEVOP_pci_device_add”, VPCI handler for the config space
access is added to the Xen to emulate the PCI devices config space.

A MMIO trap handler for the PCI ECAM space is registered in XEN
so that when guest is trying to access the PCI config space,XEN
will trap the access and emulate read/write using the VPCI and
not the real PCI hardware.

For Dom0less systems scan_pci_devices() would be used to discover the
PCI device in XEN and VPCI handler will be added during XEN boots.

This patch is also doing some small fixes to fix compilation errors on
arm32 of vpci and prevent 64bit accesses on 32bit:
- use %zu instead of lu in header.c for print
- prevent 64bit accesses in vpci_access_allowed
- ifdef out using CONFIG_64BIT handling of len 8 in

Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

The vpci bits looks fine to me, so:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>


I have one question however related to the placement of the vpci setup
call in pci_add_device.

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 3aa8c3175f..082892c8a2 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         list_add(&pdev->domain_list, &hardware_domain->pdev_list);
+    {
+#ifdef CONFIG_ARM
+        /*
+         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
+         * when Dom0 inform XEN to add the PCI devices in XEN.
+         */
+        ret = vpci_add_handlers(pdev);
+        if ( ret )
+        {
+            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
+            goto out;
+        }

I'm likely lost here, but shouldn't this also be done for devices that
belong to the hardware domain and are assigned to it in the first
branch of this conditional?

Or else you will end up with devices assigned to the hardware domain
that don't have vPCI setup for them.

I might be wrong but when the hardware domain is declaring the devices they are 
added to him.
Then later when those device are assigned to a guest, they are removed from the 
hardware domain.

From my understanding, when the device is initially registered we would go through the first branch because pdev->domain is not yet set.

The else would be taken only with subsequent call of PHYSDEVOP_manage_pci_add & co.

For the device assignment, a different path would be taken. This would go through the domctl XEN_DOMCTL_assign_device.

Therefore, I think Roger is right and the call belongs to the first branch. Otherwise, we would miss out the vpci handlers in some cases.


Julien Grall



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