Hi, Jan!
Sorry for top posting, but this is a general question on this
patch/functionality.
Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
as this renders in somewhat dead code for x86 for now? I do think this still
needs to be in the common code though.
Thank you in advance,
Oleksandr
On 28.09.21 15:58, Oleksandr Andrushchenko wrote:
> On 28.09.21 11:17, Michal Orzel wrote:
>> On 28.09.2021 09:59, Jan Beulich wrote:
>>> On 28.09.2021 09:48, Michal Orzel wrote:
>>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
>>>>> + const struct pci_dev
>>>>> *pdev)
>>>>> +{
>>>>> + struct vpci_dev *vdev;
>>>>> +
>>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>>> + if ( vdev->pdev == pdev )
>>>>> + return vdev;
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>>> +{
>>>>> + struct vpci_dev *vdev;
>>>>> +
>>>>> + ASSERT(!pci_find_virtual_device(d, pdev));
>>>>> +
>>>>> + /* Each PCI bus supports 32 devices/slots at max. */
>>>>> + if ( d->vpci_dev_next > 31 )
>>>>> + return -ENOSPC;
>>>>> +
>>>>> + vdev = xzalloc(struct vpci_dev);
>>>>> + if ( !vdev )
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + /* We emulate a single host bridge for the guest, so segment is
>>>>> always 0. */
>>>>> + *(u16*) &vdev->seg = 0;
>>>> Empty line hear would improve readability due to the asterisks being so
>>>> close to each other.
> Will add
>>>> Apart from that:
>>>> Reviewed-by: Michal Orzel <michal.or...@arm.com>
>>>>> + /*
>>>>> + * The bus number is set to 0, so virtual devices are seen
>>>>> + * as embedded endpoints behind the root complex.
>>>>> + */
>>>>> + *((u8*) &vdev->bus) = 0;
>>>>> + *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
>>> All of these casts are (a) malformed and (b) unnecessary in the first
>>> place, afaics at least.
>>>
>> Agree.
>> *((u8*) &vdev->bus) = 0;
>> is the same as:
>> vdev->bus = 0;
> Overengineering at its best ;)
>
> Will fix that
>
>>> Jan
>>>
> Thank you,
>
> Oleksandr
