Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 15 Oct 2021, at 11:19, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> On Fri, Oct 15, 2021 at 09:52:28AM +0000, Bertrand Marquis wrote:
>> Hi Roger,
>> 
>>> On 15 Oct 2021, at 09:32, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>>> 
>>> On Thu, Oct 14, 2021 at 03:49:50PM +0100, Bertrand Marquis wrote:
>>>> From: Rahul Singh <rahul.singh@xxxxxxx>
>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>> index 3aa8c3175f..8cc529ecec 100644
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -658,7 +658,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>                   const struct pci_dev_info *info, nodeid_t node)
>>>> {
>>>>    struct pci_seg *pseg;
>>>> -    struct pci_dev *pdev;
>>>> +    struct pci_dev *pdev = NULL;
>>>>    unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
>>>>    const char *pdev_type;
>>>>    int ret;
>>>> @@ -752,6 +752,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>> 
>>>>    check_pdev(pdev);
>>>> 
>>>> +#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;
>>>> +    }
>>>> +#endif
>>> 
>>> I think vpci_add_handlers should be called after checking that
>>> pdev->domain is != NULL, so I would move this chunk a bit below.
>> 
>> On arm this would prevent the dom0less use case or to have the PCI
>> bus enumerated from an other domain.
> 
> vpci_add_handlers will try to access pdev->domain, so passing a pdev
> without domain being set is wrong.

Right and the initial comment from Rahul in the code is saying so.
I will move the block inside the if.

> 
>> @oleksandr: can you comment on this one, you might have a better
>> answer than me on this ?
>> 
>>> 
>>>> +
>>>>    ret = 0;
>>>>    if ( !pdev->domain )
>>>>    {
>>>> @@ -784,6 +797,9 @@ out:
>>>>                   &PCI_SBDF(seg, bus, slot, func));
>>>>        }
>>>>    }
>>>> +    else if ( pdev )
>>>> +        pci_cleanup_msi(pdev);
>>> 
>>> I'm slightly lost at why you add this chunk, is this strictly related
>>> to the patch?
>> 
>> This was discussed a lot in previous version of the patch and
>> requested by Stefano. The idea here is that as soon as handlers
>> are added some bits might be modified in the PCI config space
>> leading possibly to msi interrupts. So it is safer to cleanup on the
>> error path. For references please see discussion on v4 and v5 where
>> this was actually added (to much references as the discussion was
>> long so here [1] and [2] are the patchwork thread).
> 
> pci_add_device being solely used by trusted domains, I think it would
> be fine to require that the domain doesn't poke the PCI config space
> until the call to pci_add_device has finished. Else it's likely to get
> inconsistent results, or mess up with the device state.

Ack.

> 
> In any case, such addition needs some kind of reasoning added to the
> commit message if it's really required.

With the code moving inside the following else and pci_cleanup_msi being
empty for now on arm, I will remove the pci_cleanup_msi as it does not
related directly to this change for now and might change the behaviour on x86.

If this is needed at some point due to arm, this will be done once msi support 
will be added.

Regards
Bertrand

> 
> Thanks, Roger.