Hello Jan,
> On 28 Oct 2020, at 11:51 am, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 26.10.2020 18:17, Rahul Singh wrote:
>> passthrough/pci.c file is common for all architecture, but there is x86
>> sepcific code in this file.
>
> The code you move doesn't look to be x86 specific in the sense that
> it makes no sense on other architectures, but just because certain
> pieces are missing on Arm. With this I question whether is it really
> appropriate to move this code. I do realize that in similar earlier
> cases my questioning was mostly ignored ...
>
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/x86/pci.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> with
>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/param.h>
>> +#include <xen/sched.h>
>> +#include <xen/pci.h>
>> +#include <xen/pci_regs.h>
>> +
>> +static int pci_clean_dpci_irq(struct domain *d,
>> + struct hvm_pirq_dpci *pirq_dpci, void *arg)
>> +{
>> + struct dev_intx_gsi_link *digl, *tmp;
>> +
>> + pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
>> +
>> + if ( pt_irq_need_timer(pirq_dpci->flags) )
>> + kill_timer(&pirq_dpci->timer);
>> +
>> + list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
>> + {
>> + list_del(&digl->list);
>> + xfree(digl);
>> + }
>> +
>> + radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
>> +
>> + if ( !pt_pirq_softirq_active(pirq_dpci) )
>> + return 0;
>> +
>> + domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
>> +
>> + return -ERESTART;
>> +}
>> +
>> +static int pci_clean_dpci_irqs(struct domain *d)
>> +{
>> + struct hvm_irq_dpci *hvm_irq_dpci = NULL;
>> +
>> + if ( !is_iommu_enabled(d) )
>> + return 0;
>> +
>> + if ( !is_hvm_domain(d) )
>> + return 0;
>> +
>> + spin_lock(&d->event_lock);
>> + hvm_irq_dpci = domain_get_irq_dpci(d);
>> + if ( hvm_irq_dpci != NULL )
>> + {
>> + int ret = 0;
>> +
>> + if ( hvm_irq_dpci->pending_pirq_dpci )
>> + {
>> + if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
>> + ret = -ERESTART;
>> + else
>> + hvm_irq_dpci->pending_pirq_dpci = NULL;
>> + }
>> +
>> + if ( !ret )
>> + ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
>> + if ( ret )
>> + {
>> + spin_unlock(&d->event_lock);
>> + return ret;
>> + }
>> +
>> + hvm_domain_irq(d)->dpci = NULL;
>> + free_hvm_irq_dpci(hvm_irq_dpci);
>> + }
>> + spin_unlock(&d->event_lock);
>> + return 0;
>
> While moving please add the missing blank line before the main
> return statement of the function.
Ok I will fix that in next version.
>
>> +}
>> +
>> +int arch_pci_release_devices(struct domain *d)
>> +{
>> + return pci_clean_dpci_irqs(d);
>> +}
>
> Why the extra function layer?
Is that ok if I rename the function pci_clean_dpci_irqs() to
arch_pci_clean_pirqs() ?
>
> Jan
>
Regards,
Rahul
