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

From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
To: Jan Beulich <jbeulich@suse.com>,
	Oleksandr Andrushchenko <andr2000@gmail.com>
Cc: "iwj@xenproject.org" <iwj@xenproject.org>,
	"wl@xen.org" <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Bertrand.Marquis@arm.com" <Bertrand.Marquis@arm.com>,
	"julien.grall@arm.com" <julien.grall@arm.com>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>,
	"roger.pau@citrix.com" <roger.pau@citrix.com>,
	"Rahul.Singh@arm.com" <Rahul.Singh@arm.com>
Subject: Re: [PATCH 02/10] arm/pci: Maintain PCI assignable list
Date: Thu, 12 Nov 2020 12:53:47 +0000	[thread overview]
Message-ID: <eb50b7f2-a2fb-a7fb-7937-511e5b26f7a0@epam.com> (raw)
In-Reply-To: <6a2dc03e-202b-b8f9-46a5-9c90d9de8a6d@suse.com>


On 11/11/20 4:54 PM, Jan Beulich wrote:
> On 09.11.2020 13:50, Oleksandr Andrushchenko wrote:
>> --- 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));
>> +        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
> Just in case the earlier comments you've got don't lead to removal
> of this code - unless there's a real need for them to be put here,
> under #ifdef, please add a new xen/drivers/passthrough/arm/pci.c
> instead. Even if for just part of the code, this would then also
> help with more clear maintainership of this Arm specific code.
Yes, does make sense to move all ARM specifics into a dedicated file
>
> Jan