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

Re: [PATCH 02/10] arm/pci: Maintain PCI assignable list


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Date: Wed, 11 Nov 2020 14:38:47 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3qoWBtXRI8LyAe+qCmeGB5z5cb+ZyA3oeWE92XfobDo=; b=RkRkRQKmn3SMv77F/TaYO+MbqO0DEa2RgbPMOY/MSmX7RbOyB2s815HXGNR7G9nr9TjbpXHnwh8DWhqh76u1V3bc4+At9mND06NAFwYlMMrCw/CGlwfaMi9+/Z6H2cQmBtElUHv1zQpuUsIOaHvWBB+6GJBx3oEF5vPdzn6LvGMS/q9Rg1LlLy+o0a3seNoFVooBRUIcXD+rd3GZbUbAA/Kslo8QfNyy1sSjdj0tu9IUqv5pm5QP6jDsrcal9S65bn7ZVUtYsKighzgb6yNz4FoEka0JiZuUFAN4XTPbYBaKwf352Tp7XNY3/wjLPG3DGo5fGIho6GL7LQ0OOfifaw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=oQi/MRe6uYJJQiRxjM5eKeaiCux6IuhivRfA5kQAIUKNXwMGSSzon5E8+Wd8WwD4eriIgX92LgfWwzCvGyfUEUavS8+QzObOiR1O60Vzod3AiR4wn16XgySUcMM08k774e67gNN6cSOwyIbGS6NPlTmAn1arqIy8O1e/UBoCXskkJcOQjPhixirdmU8Hr32q5FCzNWRF7eNIbvOHXpWnMHemoy28Y3wNniTWG9Uny5QA7MoTBiiCr4BapTxD25pMOtFZ+8DItnEU2MorMB6C7jzesFVeJGK1Wtyeu2l1v4enVdutrgiqNtv995bJjsl9TvvvnEYIfYPZWROOB5qUxw==
  • Authentication-results: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=epam.com;
  • Cc: "Rahul.Singh@xxxxxxx" <Rahul.Singh@xxxxxxx>, "Bertrand.Marquis@xxxxxxx" <Bertrand.Marquis@xxxxxxx>, "julien.grall@xxxxxxx" <julien.grall@xxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "iwj@xxxxxxxxxxxxxx" <iwj@xxxxxxxxxxxxxx>, "wl@xxxxxxx" <wl@xxxxxxx>
  • Delivery-date: Wed, 11 Nov 2020 14:39:06 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHWtpbuKiRgL1HiZkyNC2yoNcmnl6nC9s6AgAAMvAA=
  • Thread-topic: [PATCH 02/10] arm/pci: Maintain PCI assignable list

On 11/11/20 3:53 PM, Roger Pau Monné wrote:
> On Mon, Nov 09, 2020 at 02:50:23PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> The original code depends on pciback to manage assignable device list.
>> The functionality which is implemented by the pciback and the toolstack
>> and which is relevant/missing/needed for ARM:
>>
>> 1. pciback is used as a database for assignable PCI devices, e.g. xl
>>     pci-assignable-{add|remove|list} manipulates that list. So, whenever the
>>     toolstack needs to know which PCI devices can be passed through it reads
>>     that from the relevant sysfs entries of the pciback.
>>
>> 2. pciback is used to hold the unbound PCI devices, e.g. when passing through
>>     a PCI device it needs to be unbound from the relevant device driver and 
>> bound
>>     to pciback (strictly speaking it is not required that the device is 
>> bound to
>>     pciback, but pciback is again used as a database of the passed through 
>> PCI
>>     devices, so we can re-bind the devices back to their original drivers 
>> when
>>     guest domain shuts down)
>>
>> 1. As ARM doesn't use pciback implement the above with additional sysctls:
>>   - XEN_SYSCTL_pci_device_set_assigned
> I don't see the point in having this sysfs, Xen already knows when a
> device is assigned because the XEN_DOMCTL_assign_device hypercall is
> used.

But how does the toolstack know about that? When the toolstack needs to

list/know all assigned devices it queries pciback's sysfs entries. So, with

XEN_DOMCTL_assign_device we make that knowledge available to Xen,

but there are no means for the toolstack to get it back.

>
>>   - XEN_SYSCTL_pci_device_get_assigned
>>   - XEN_SYSCTL_pci_device_enum_assigned
>> 2. Extend struct pci_dev to hold assignment state.
> I'm not really found of this, the hypervisor is no place to store a
> database like this, unless it's strictly needed.
I do agree and it was previously discussed a bit
>
> IMO the right implementation here would be to split Linux pciback into
> two different drivers:
>
>   - The pv-pci backend for doing passthrough to classic PV guests.
Ok
>   - The rest of pciback: device reset, hand-holding driver for devices
>     to be assigned and database.

These and assigned devices list seem to be the complete set which

is needed by the toolstack on ARM. All other functionality provided by

pciback is not needed for ARM.

Jan was saying [1] that we might still use pciback as is, but simply use only

the functionality we need.

>
> I think there must be something similar in KVM that performs the tasks
> of my last point, maybe we could piggyback on it?
I promised to look at it. I owe this
>
> If we want to go the route proposed by this patch, ie: Xen performing
> the functions of pciback you would also have to move the PCI reset
> code to Xen, so that you can fully manage the PCI devices from Xen.
In case of dom0less this would be the case: no pciback, no Domain-0
>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>>   tools/libxc/include/xenctrl.h |   9 +++
>>   tools/libxc/xc_domain.c       |   1 +
>>   tools/libxc/xc_misc.c         |  46 +++++++++++++++
>>   tools/libxl/Makefile          |   4 ++
>>   tools/libxl/libxl_pci.c       | 105 ++++++++++++++++++++++++++++++++--
>>   xen/arch/arm/sysctl.c         |  66 ++++++++++++++++++++-
>>   xen/drivers/passthrough/pci.c |  93 ++++++++++++++++++++++++++++++
>>   xen/include/public/sysctl.h   |  40 +++++++++++++
>>   xen/include/xen/pci.h         |  12 ++++
>>   9 files changed, 370 insertions(+), 6 deletions(-)
> I've done some light review below given my questions above.

This is more than I expected for an RFC series

Thank you!

>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 4c89b7294c4f..77029013da7d 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2652,6 +2652,15 @@ int xc_livepatch_replace(xc_interface *xch, char 
>> *name, uint32_t timeout, uint32
>>   int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
>>                            xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
>>   
>> +typedef xen_sysctl_pci_device_enum_assigned_t xc_pci_device_enum_assigned_t;
>> +
>> +int xc_pci_device_set_assigned(xc_interface *xch, uint32_t machine_sbdf,
>> +                               bool assigned);
>> +int xc_pci_device_get_assigned(xc_interface *xch, uint32_t machine_sbdf);
>> +
>> +int xc_pci_device_enum_assigned(xc_interface *xch,
>> +                                xc_pci_device_enum_assigned_t *e);
>> +
>>   /* Compat shims */
>>   #include "xenctrl_compat.h"
>>   
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index 71829c2bce3e..d515191e9243 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -2321,6 +2321,7 @@ int xc_domain_soft_reset(xc_interface *xch,
>>       domctl.domain = domid;
>>       return do_domctl(xch, &domctl);
>>   }
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
>> index 3820394413a9..d439c4ba1019 100644
>> --- a/tools/libxc/xc_misc.c
>> +++ b/tools/libxc/xc_misc.c
>> @@ -988,6 +988,52 @@ int xc_livepatch_replace(xc_interface *xch, char *name, 
>> uint32_t timeout, uint32
>>       return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, 
>> timeout, flags);
>>   }
>>   
>> +int xc_pci_device_set_assigned(
>> +    xc_interface *xch,
>> +    uint32_t machine_sbdf,
>> +    bool assigned)
>> +{
>> +    DECLARE_SYSCTL;
>> +
>> +    sysctl.cmd = XEN_SYSCTL_pci_device_set_assigned;
>> +    sysctl.u.pci_set_assigned.machine_sbdf = machine_sbdf;
>> +    sysctl.u.pci_set_assigned.assigned = assigned;
>> +
>> +    return do_sysctl(xch, &sysctl);
>> +}
>> +
>> +int xc_pci_device_get_assigned(
>> +    xc_interface *xch,
>> +    uint32_t machine_sbdf)
>> +{
>> +    DECLARE_SYSCTL;
>> +
>> +    sysctl.cmd = XEN_SYSCTL_pci_device_get_assigned;
>> +    sysctl.u.pci_get_assigned.machine_sbdf = machine_sbdf;
>> +
>> +    return do_sysctl(xch, &sysctl);
>> +}
>> +
>> +int xc_pci_device_enum_assigned(xc_interface *xch,
>> +                                xc_pci_device_enum_assigned_t *e)
>> +{
>> +    int ret;
>> +    DECLARE_SYSCTL;
>> +
>> +    sysctl.cmd = XEN_SYSCTL_pci_device_enum_assigned;
>> +    sysctl.u.pci_enum_assigned.idx = e->idx;
>> +    sysctl.u.pci_enum_assigned.report_not_assigned = e->report_not_assigned;
>> +    ret = do_sysctl(xch, &sysctl);
>> +    if ( ret )
>> +        errno = EINVAL;
>> +    else
>> +    {
>> +        e->domain = sysctl.u.pci_enum_assigned.domain;
>> +        e->machine_sbdf = sysctl.u.pci_enum_assigned.machine_sbdf;
>> +    }
>> +    return ret;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index f3806aafcb4e..6f76ba35aec7 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -130,6 +130,10 @@ endif
>>   
>>   LIBXL_LIBS += -lyajl
>>   
>> +ifeq ($(CONFIG_X86),y)
>> +CFALGS += -DCONFIG_PCIBACK
>> +endif
>> +
>>   ifeq ($(CONFIG_ARM),y)
>>   CFALGS += -DCONFIG_ARM
>>   endif
>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
>> index b93cf976642b..41f89b8aae10 100644
>> --- a/tools/libxl/libxl_pci.c
>> +++ b/tools/libxl/libxl_pci.c
>> @@ -319,6 +319,7 @@ retry_transaction2:
>>   
>>   static int get_all_assigned_devices(libxl__gc *gc, libxl_device_pci 
>> **list, int *num)
>>   {
>> +#ifdef CONFIG_PCIBACK
>>       char **domlist;
>>       unsigned int nd = 0, i;
>>   
>> @@ -356,6 +357,33 @@ static int get_all_assigned_devices(libxl__gc *gc, 
>> libxl_device_pci **list, int
>>               }
>>           }
>>       }
>> +#else
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +    int ret;
>> +    xc_pci_device_enum_assigned_t e;
>> +
>> +    *list = NULL;
>> +    *num = 0;
>> +
>> +    memset(&e, 0, sizeof(e));
>> +    do {
>> +        ret = xc_pci_device_enum_assigned(ctx->xch, &e);
>> +        if ( ret && errno == EINVAL )
>> +            break;
>> +        *list = realloc(*list, sizeof(libxl_device_pci) * (e.idx + 1));
>> +        if (*list == NULL)
>> +            return ERROR_NOMEM;
>> +
>> +        pcidev_struct_fill(*list + e.idx,
>> +                           e.domain,
>> +                           e.machine_sbdf >> 8 & 0xff,
>> +                           PCI_SLOT(e.machine_sbdf),
>> +                           PCI_FUNC(e.machine_sbdf),
>> +                           0 /*vdevfn*/);
>> +        e.idx++;
>> +    } while (!ret);
>> +    *num = e.idx;
>> +#endif
> I don't think the amount of ifdefs added to this file is acceptable.
> If we have to go that route this needs to be split into a different
> file, and maybe some of the common bits abstracted together to prevent
> code repetition.

We also briefly discussed that and were talking about if the arch specific

files should be someting like libxl_pci_x86_linux.c etc.

>
>>       libxl__ptr_add(gc, *list);
>>   
>>       return 0;
>> @@ -411,13 +439,20 @@ static int sysfs_write_bdf(libxl__gc *gc, const char * 
>> sysfs_path,
>>   libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int 
>> *num)
>>   {
>>       GC_INIT(ctx);
>> -    libxl_device_pci *pcidevs = NULL, *new, *assigned;
>> +    libxl_device_pci *pcidevs = NULL, *new;
>> +    int r;
>> +#ifdef CONFIG_PCIBACK
>> +    libxl_device_pci *assigned;
>> +    int num_assigned;
>>       struct dirent *de;
>>       DIR *dir;
>> -    int r, num_assigned;
>> +#else
>> +    xc_pci_device_enum_assigned_t e;
>> +#endif
>>   
>>       *num = 0;
>>   
>> +#ifdef CONFIG_PCIBACK
>>       r = get_all_assigned_devices(gc, &assigned, &num_assigned);
>>       if (r) goto out;
>>   
>> @@ -453,6 +488,32 @@ libxl_device_pci 
>> *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
>>   
>>       closedir(dir);
>>   out:
>> +#else
>> +    memset(&e, 0, sizeof(e));
>> +    e.report_not_assigned = 1;
>> +    do {
>> +        r = xc_pci_device_enum_assigned(ctx->xch, &e);
>> +        if ( r && errno == EINVAL )
>> +            break;
>> +        new = realloc(pcidevs, (e.idx + 1) * sizeof(*new));
>> +        if (NULL == new)
>> +            continue;
>> +
>> +        pcidevs = new;
>> +        new = pcidevs + e.idx;
>> +
>> +        memset(new, 0, sizeof(*new));
>> +
>> +        pcidev_struct_fill(new,
>> +                           e.domain,
>> +                           e.machine_sbdf >> 8 & 0xff,
>> +                           PCI_SLOT(e.machine_sbdf),
>> +                           PCI_FUNC(e.machine_sbdf),
>> +                           0 /*vdevfn*/);
>> +        e.idx++;
>> +    } while (!r);
>> +    *num = e.idx;
>> +#endif
>>       GC_FREE;
>>       return pcidevs;
>>   }
>> @@ -606,6 +667,7 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
>>       return false;
>>   }
>>   
>> +#ifdef CONFIG_PCIBACK
>>   /*
>>    * A brief comment about slots.  I don't know what slots are for; however,
>>    * I have by experimentation determined:
>> @@ -648,11 +710,13 @@ out:
>>       fclose(f);
>>       return rc;
>>   }
>> +#endif
>>   
>>   static int pciback_dev_is_assigned(libxl__gc *gc, libxl_device_pci *pcidev)
>>   {
>> -    char * spath;
>>       int rc;
>> +#ifdef CONFIG_PCIBACK
>> +    char * spath;
>>       struct stat st;
>>   
>>       if ( access(SYSFS_PCIBACK_DRIVER, F_OK) < 0 ) {
>> @@ -663,22 +727,27 @@ static int pciback_dev_is_assigned(libxl__gc *gc, 
>> libxl_device_pci *pcidev)
>>           }
>>           return -1;
>>       }
>> -
>>       spath = GCSPRINTF(SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
>>                         pcidev->domain, pcidev->bus,
>>                         pcidev->dev, pcidev->func);
>>       rc = lstat(spath, &st);
>> -
>>       if( rc == 0 )
>>           return 1;
>>       if ( rc < 0 && errno == ENOENT )
>>           return 0;
>>       LOGE(ERROR, "Accessing %s", spath);
>>       return -1;
>> +#else
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +
>> +    rc = xc_pci_device_get_assigned(ctx->xch, pcidev_encode_bdf(pcidev));
>> +    return rc == 0 ? 1 : 0;
>> +#endif
>>   }
>>   
>>   static int pciback_dev_assign(libxl__gc *gc, libxl_device_pci *pcidev)
>>   {
>> +#ifdef CONFIG_PCIBACK
>>       int rc;
>>   
>>       if ( (rc=pciback_dev_has_slot(gc, pcidev)) < 0 ) {
>> @@ -697,10 +766,17 @@ static int pciback_dev_assign(libxl__gc *gc, 
>> libxl_device_pci *pcidev)
>>           return ERROR_FAIL;
>>       }
>>       return 0;
>> +#else
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +
>> +    return xc_pci_device_set_assigned(ctx->xch, pcidev_encode_bdf(pcidev),
>> +                                      true);
>> +#endif
>>   }
>>   
>>   static int pciback_dev_unassign(libxl__gc *gc, libxl_device_pci *pcidev)
>>   {
>> +#ifdef CONFIG_PCIBACK
>>       /* Remove from pciback */
>>       if ( sysfs_dev_unbind(gc, pcidev, NULL) < 0 ) {
>>           LOG(ERROR, "Couldn't unbind device!");
>> @@ -716,6 +792,12 @@ static int pciback_dev_unassign(libxl__gc *gc, 
>> libxl_device_pci *pcidev)
>>           }
>>       }
>>       return 0;
>> +#else
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +
>> +    return xc_pci_device_set_assigned(ctx->xch, pcidev_encode_bdf(pcidev),
>> +                                      false);
>> +#endif
>>   }
>>   
>>   #define PCIBACK_INFO_PATH "/libxl/pciback"
>> @@ -780,10 +862,15 @@ static int libxl__device_pci_assignable_add(libxl__gc 
>> *gc,
>>   
>>       /* See if the device exists */
>>       spath = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF, dom, bus, dev, func);
>> +#ifdef CONFIG_PCI_SYSFS_DOM0
>>       if ( lstat(spath, &st) ) {
>>           LOGE(ERROR, "Couldn't lstat %s", spath);
>>           return ERROR_FAIL;
>>       }
>> +#else
>> +    (void)st;
>> +    printf("IMPLEMENT_ME: %s lstat %s\n", __func__, spath);
>> +#endif
>>   
>>       /* Check to see if it's already assigned to pciback */
>>       rc = pciback_dev_is_assigned(gc, pcidev);
>> @@ -1350,8 +1437,12 @@ static void pci_add_dm_done(libxl__egc *egc,
>>   
>>       if (f == NULL) {
>>           LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>> +#ifdef CONFIG_PCI_SYSFS_DOM0
>>           rc = ERROR_FAIL;
>>           goto out;
>> +#else
>> +        goto out_no_irq;
>> +#endif
>>       }
>>       for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
>>           if (fscanf(f, "0x%llx 0x%llx 0x%llx\n", &start, &end, &flags) != 3)
>> @@ -1522,7 +1613,11 @@ static int libxl_pcidev_assignable(libxl_ctx *ctx, 
>> libxl_device_pci *pcidev)
>>               break;
>>       }
>>       free(pcidevs);
>> +#ifdef CONFIG_PCIBACK
>>       return i != num;
>> +#else
>> +    return 1;
>> +#endif
>>   }
>>   
>>   static void device_pci_add_stubdom_wait(libxl__egc *egc,
>> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
>> index f87944e8473c..84e933b2eb45 100644
>> --- a/xen/arch/arm/sysctl.c
>> +++ b/xen/arch/arm/sysctl.c
>> @@ -10,6 +10,7 @@
>>   #include <xen/lib.h>
>>   #include <xen/errno.h>
>>   #include <xen/hypercall.h>
>> +#include <xen/guest_access.h>
>>   #include <public/sysctl.h>
>>   
>>   void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>> @@ -20,7 +21,70 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>   long arch_do_sysctl(struct xen_sysctl *sysctl,
>>                       XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>   {
>> -    return -ENOSYS;
>> +    long ret = 0;
>> +    bool copyback = 0;
>> +
>> +    switch ( sysctl->cmd )
>> +    {
>> +    case XEN_SYSCTL_pci_device_set_assigned:
>> +    {
>> +        u16 seg;
>> +        u8 bus, devfn;
>> +        uint32_t machine_sbdf;
>> +
>> +        machine_sbdf = sysctl->u.pci_set_assigned.machine_sbdf;
>> +
>> +#if 0
>> +        ret = xsm_pci_device_set_assigned(XSM_HOOK, d);
>> +        if ( ret )
>> +            break;
>> +#endif
>> +
>> +        seg = machine_sbdf >> 16;
>> +        bus = PCI_BUS(machine_sbdf);
>> +        devfn = PCI_DEVFN2(machine_sbdf);
>> +
>> +        pcidevs_lock();
>> +        ret = pci_device_set_assigned(seg, bus, devfn,
>> +                                      
>> !!sysctl->u.pci_set_assigned.assigned);
>> +        pcidevs_unlock();
>> +        break;
>> +    }
>> +    case XEN_SYSCTL_pci_device_get_assigned:
>> +    {
>> +        u16 seg;
>> +        u8 bus, devfn;
>> +        uint32_t machine_sbdf;
>> +
>> +        machine_sbdf = sysctl->u.pci_set_assigned.machine_sbdf;
>> +
>> +        seg = machine_sbdf >> 16;
>> +        bus = PCI_BUS(machine_sbdf);
>> +        devfn = PCI_DEVFN2(machine_sbdf);
>> +
>> +        pcidevs_lock();
>> +        ret = pci_device_get_assigned(seg, bus, devfn);
>> +        pcidevs_unlock();
>> +        break;
>> +    }
>> +    case XEN_SYSCTL_pci_device_enum_assigned:
>> +    {
>> +        ret = 
>> pci_device_enum_assigned(sysctl->u.pci_enum_assigned.report_not_assigned,
>> +                                       sysctl->u.pci_enum_assigned.idx,
>> +                                       &sysctl->u.pci_enum_assigned.domain,
>> +                                       
>> &sysctl->u.pci_enum_assigned.machine_sbdf);
>> +        copyback = 1;
>> +        break;
>> +    }
>> +    default:
>> +        ret = -ENOSYS;
>> +        break;
>> +    }
>> +    if ( copyback && (!ret || copyback > 0) &&
>> +         __copy_to_guest(u_sysctl, sysctl, 1) )
>> +        ret = -EFAULT;
>> +
>> +    return ret;
>>   }
>>   
>>   /*
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 98e8a2fade60..49b4279c63bd 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -879,6 +879,43 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>       return ret;
>>   }
>>   
>> +#ifdef CONFIG_ARM
>> +int pci_device_set_assigned(u16 seg, u8 bus, u8 devfn, bool assigned)
>> +{
>> +    struct pci_dev *pdev;
>> +
>> +    pdev = pci_get_pdev(seg, bus, devfn);
>> +    if ( !pdev )
>> +    {
>> +        printk(XENLOG_ERR "Can't find PCI device %04x:%02x:%02x.%u\n",
>> +               seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> Take a look at pci_sbdf_t, you should use it as the parameter and in
> order to print the SBDF (%pp).
I will, thank you
>
>> +        return -ENODEV;
>> +    }
>> +
>> +    pdev->assigned = assigned;
>> +    printk(XENLOG_ERR "pciback %sassign PCI device %04x:%02x:%02x.%u\n",
>> +           assigned ? "" : "de-",
>> +           seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +
>> +    return 0;
>> +}
>> +
>> +int pci_device_get_assigned(u16 seg, u8 bus, u8 devfn)
>> +{
>> +    struct pci_dev *pdev;
>> +
>> +    pdev = pci_get_pdev(seg, bus, devfn);
>> +    if ( !pdev )
>> +    {
>> +        printk(XENLOG_ERR "Can't find PCI device %04x:%02x:%02x.%u\n",
>> +               seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +        return -ENODEV;
>> +    }
>> +
>> +    return pdev->assigned ? 0 : -ENODEV;
>> +}
>> +#endif
>> +
>>   #ifndef CONFIG_ARM
>>   /*TODO :Implement MSI support for ARM  */
>>   static int pci_clean_dpci_irq(struct domain *d,
>> @@ -1821,6 +1858,62 @@ int iommu_do_pci_domctl(
>>       return ret;
>>   }
>>   
>> +#ifdef CONFIG_ARM
>> +struct list_assigned {
>> +    uint32_t cur_idx;
>> +    uint32_t from_idx;
>> +    bool assigned;
>> +    domid_t *domain;
>> +    uint32_t *machine_sbdf;
>> +};
>> +
>> +static int _enum_assigned_pci_devices(struct pci_seg *pseg, void *arg)
>> +{
>> +    struct list_assigned *ctxt = arg;
>> +    struct pci_dev *pdev;
>> +
>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> +    {
>> +        if ( pdev->assigned == ctxt->assigned )
>> +        {
>> +            if ( ctxt->cur_idx == ctxt->from_idx )
>> +            {
>> +                *ctxt->domain = pdev->domain->domain_id;
>> +                *ctxt->machine_sbdf = pdev->sbdf.sbdf;
>> +                return 1;
>> +            }
>> +            ctxt->cur_idx++;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +int pci_device_enum_assigned(bool report_not_assigned,
>> +                             uint32_t from_idx, domid_t *domain,
>> +                             uint32_t *machine_sbdf)
>> +{
>> +    struct list_assigned ctxt = {
>> +        .assigned = !report_not_assigned,
>> +        .cur_idx = 0,
>> +        .from_idx = from_idx,
>> +        .domain = domain,
>> +        .machine_sbdf = machine_sbdf,
>> +    };
>> +    int ret;
>> +
>> +    pcidevs_lock();
>> +    ret = pci_segments_iterate(_enum_assigned_pci_devices, &ctxt);
>> +    pcidevs_unlock();
>> +    /*
>> +     * If not found then report as EINVAL to mark
>> +     * enumeration process finished.
>> +     */
>> +    if ( !ret )
>> +        return -EINVAL;
>> +    return 0;
>> +}
>> +#endif
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index a07364711794..5ca73c538688 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1062,6 +1062,40 @@ typedef struct xen_sysctl_cpu_policy 
>> xen_sysctl_cpu_policy_t;
>>   DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
>>   #endif
>>   
>> +/*
>> + * These are to emulate pciback device (de-)assignment used by the tools
>> + * to track current device assignments: all the PCI devices that can
>> + * be passed through must be assigned to the pciback to mark them
>> + * as such. As on ARM we do not run pci{back|front} and are emulating
>> + * PCI host bridge in Xen, so we need to maintain the assignments on our
>> + * own in Xen itself.
>> + *
>> + * Note on xen_sysctl_pci_device_get_assigned: ENOENT is used to report
>> + * that there are no assigned devices left.
>> + */
>> +struct xen_sysctl_pci_device_set_assigned {
>> +    /* IN */
>> +    /* FIXME: is this really a machine SBDF or as Domain-0 sees it? */
>> +    uint32_t machine_sbdf;
> I think you need to make it clear that when running on Xen dom0 (or
> the hardware domain) should _never_ change the enumeration of devices,
> or else none of this will work.
I will
>
>> +    uint8_t assigned;
>> +};
>> +
>> +struct xen_sysctl_pci_device_get_assigned {
>> +    /* IN */
>> +    uint32_t machine_sbdf;
>> +};
>> +
>> +struct xen_sysctl_pci_device_enum_assigned {
>> +    /* IN */
>> +    uint32_t idx;
>> +    uint8_t report_not_assigned;
>> +    /* OUT */
>> +    domid_t domain;
>> +    uint32_t machine_sbdf;
>> +};
>> +typedef struct xen_sysctl_pci_device_enum_assigned 
>> xen_sysctl_pci_device_enum_assigned_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pci_device_enum_assigned_t);
>> +
>>   struct xen_sysctl {
>>       uint32_t cmd;
>>   #define XEN_SYSCTL_readconsole                    1
>> @@ -1092,6 +1126,9 @@ struct xen_sysctl {
>>   #define XEN_SYSCTL_livepatch_op                  27
>>   /* #define XEN_SYSCTL_set_parameter              28 */
>>   #define XEN_SYSCTL_get_cpu_policy                29
>> +#define XEN_SYSCTL_pci_device_set_assigned       30
>> +#define XEN_SYSCTL_pci_device_get_assigned       31
>> +#define XEN_SYSCTL_pci_device_enum_assigned      32
>>       uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>>       union {
>>           struct xen_sysctl_readconsole       readconsole;
>> @@ -1122,6 +1159,9 @@ struct xen_sysctl {
>>   #if defined(__i386__) || defined(__x86_64__)
>>           struct xen_sysctl_cpu_policy        cpu_policy;
>>   #endif
>> +        struct xen_sysctl_pci_device_set_assigned pci_set_assigned;
>> +        struct xen_sysctl_pci_device_get_assigned pci_get_assigned;
>> +        struct xen_sysctl_pci_device_enum_assigned pci_enum_assigned;
>>           uint8_t                             pad[128];
>>       } u;
>>   };
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index 2bc4aaf4530c..7bf439de4de0 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -132,6 +132,13 @@ struct pci_dev {
>>   
>>       /* Data for vPCI. */
>>       struct vpci *vpci;
>> +#ifdef CONFIG_ARM
>> +    /*
>> +     * Set if this PCI device is eligible for pass through,
>> +     * e.g. just like it was assigned to pciback driver.
>> +     */
>> +    bool assigned;
> You can see whether a device is assigned or not by looking at the
> domain field AFAICT.

Hm, domain field could be dom_io, so we need to put an extra logic here

to understand the device is really assigned to the domain

>
> Thanks, Roger.

Thank you!

Oleksandr

[1] https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg77422.html

 


Rackspace

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