1. Patchew
  2. Xen
  3. [PATCH v2 00/11] PCI devices passthrough on Arm, part 2
  4. View patch

[PATCH v2 05/11] xen/arm: Mark device as PCI while creating one

Oleksandr Andrushchenko posted 11 patches 3 years, 2 months ago
There is a newer version of this series
[PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

While adding a PCI device mark it as such, so other frameworks
can distinguish it form DT devices.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

---
Since v1:
 - Moved the assignment from iommu_add_device to alloc_pdev
---
 xen/drivers/passthrough/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 633e89ac1311..fc3469bc12dc 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
     *((u8*) &pdev->bus) = bus;
     *((u8*) &pdev->devfn) = devfn;
     pdev->domain = NULL;
+#ifdef CONFIG_ARM
+    pci_to_dev(pdev)->type = DEV_PCI;
+#endif
 
     rc = pdev_msi_init(pdev);
     if ( rc )
-- 
2.25.1
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Stefano Stabellini 3 years, 2 months ago 
On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> While adding a PCI device mark it as such, so other frameworks
> can distinguish it form DT devices.
                     ^ from

> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Since v1:
>  - Moved the assignment from iommu_add_device to alloc_pdev
> ---
>  xen/drivers/passthrough/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 633e89ac1311..fc3469bc12dc 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>      *((u8*) &pdev->bus) = bus;
>      *((u8*) &pdev->devfn) = devfn;
>      pdev->domain = NULL;
> +#ifdef CONFIG_ARM
> +    pci_to_dev(pdev)->type = DEV_PCI;
> +#endif
>  
>      rc = pdev_msi_init(pdev);
>      if ( rc )
> -- 
> 2.25.1
>
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 25.09.21 02:54, Stefano Stabellini wrote:
> On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> While adding a PCI device mark it as such, so other frameworks
>> can distinguish it form DT devices.
>                       ^ from
Will fix
>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
>
>> ---
>> Since v1:
>>   - Moved the assignment from iommu_add_device to alloc_pdev
>> ---
>>   xen/drivers/passthrough/pci.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 633e89ac1311..fc3469bc12dc 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>       *((u8*) &pdev->bus) = bus;
>>       *((u8*) &pdev->devfn) = devfn;
>>       pdev->domain = NULL;
>> +#ifdef CONFIG_ARM
>> +    pci_to_dev(pdev)->type = DEV_PCI;
>> +#endif
>>   
>>       rc = pdev_msi_init(pdev);
>>       if ( rc )
>> -- 
>> 2.25.1
>>
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Jan Beulich 3 years, 2 months ago 
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>      *((u8*) &pdev->bus) = bus;
>      *((u8*) &pdev->devfn) = devfn;
>      pdev->domain = NULL;
> +#ifdef CONFIG_ARM
> +    pci_to_dev(pdev)->type = DEV_PCI;
> +#endif

I have to admit that I'm not happy about new CONFIG_<arch> conditionals
here. I'd prefer to see this done by a new arch helper, unless there are
obstacles I'm overlooking.

Jan
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 27.09.21 10:45, Jan Beulich wrote:
> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>       *((u8*) &pdev->bus) = bus;
>>       *((u8*) &pdev->devfn) = devfn;
>>       pdev->domain = NULL;
>> +#ifdef CONFIG_ARM
>> +    pci_to_dev(pdev)->type = DEV_PCI;
>> +#endif
> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
> here. I'd prefer to see this done by a new arch helper, unless there are
> obstacles I'm overlooking.

Do you mean something like arch_pci_alloc_pdev(dev)?

If so, where should we put this call? At the very beginning of alloc_pdev

or at the bottom just before returning from alloc_pdev?

>
> Jan
>
Thank you,

Oleksandr
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Jan Beulich 3 years, 2 months ago 
On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
> 
> On 27.09.21 10:45, Jan Beulich wrote:
>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>       *((u8*) &pdev->bus) = bus;
>>>       *((u8*) &pdev->devfn) = devfn;
>>>       pdev->domain = NULL;
>>> +#ifdef CONFIG_ARM
>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>> +#endif
>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>> here. I'd prefer to see this done by a new arch helper, unless there are
>> obstacles I'm overlooking.
> 
> Do you mean something like arch_pci_alloc_pdev(dev)?

I'd recommend against "alloc" in its name; "new" instead maybe?

> If so, where should we put this call? At the very beginning of alloc_pdev
> or at the bottom just before returning from alloc_pdev?

Right where you have the #ifdef right now, I would say (separated by
a blank line).

Jan
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 27.09.21 12:19, Jan Beulich wrote:
> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>> On 27.09.21 10:45, Jan Beulich wrote:
>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>        *((u8*) &pdev->bus) = bus;
>>>>        *((u8*) &pdev->devfn) = devfn;
>>>>        pdev->domain = NULL;
>>>> +#ifdef CONFIG_ARM
>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>> +#endif
>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>> obstacles I'm overlooking.
>> Do you mean something like arch_pci_alloc_pdev(dev)?
> I'd recommend against "alloc" in its name; "new" instead maybe?

I am fine with arch_pci_new_pdev, but arch prefix points to the fact that

this is just an architecture specific part of the pdev allocation rather than

actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems

more natural to me.

>
>> If so, where should we put this call? At the very beginning of alloc_pdev
>> or at the bottom just before returning from alloc_pdev?
> Right where you have the #ifdef right now, I would say (separated by
> a blank line).
Ok
>
> Jan
>
>
Thank you,

Oleksandr
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Jan Beulich 3 years, 2 months ago 
On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
> 
> On 27.09.21 12:19, Jan Beulich wrote:
>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>        *((u8*) &pdev->bus) = bus;
>>>>>        *((u8*) &pdev->devfn) = devfn;
>>>>>        pdev->domain = NULL;
>>>>> +#ifdef CONFIG_ARM
>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>> +#endif
>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>> obstacles I'm overlooking.
>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>> I'd recommend against "alloc" in its name; "new" instead maybe?
> 
> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
> this is just an architecture specific part of the pdev allocation rather than
> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
> more natural to me.

The bulk of the function is about populating the just allocated struct.
There's no arch-specific part of the allocation (so far, leaving aside
MSI-X), you only want and arch-specific part of the initialization. I
would agree with "alloc" in the name if further allocation was to
happen there.

Jan
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 27.09.21 13:00, Jan Beulich wrote:
> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>> On 27.09.21 12:19, Jan Beulich wrote:
>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>         *((u8*) &pdev->bus) = bus;
>>>>>>         *((u8*) &pdev->devfn) = devfn;
>>>>>>         pdev->domain = NULL;
>>>>>> +#ifdef CONFIG_ARM
>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>> +#endif
>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>> obstacles I'm overlooking.
>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>> this is just an architecture specific part of the pdev allocation rather than
>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>> more natural to me.
> The bulk of the function is about populating the just allocated struct.
> There's no arch-specific part of the allocation (so far, leaving aside
> MSI-X), you only want and arch-specific part of the initialization. I
> would agree with "alloc" in the name if further allocation was to
> happen there.
Hm, then arch_pci_init_pdev sounds more reasonable
> Jan
>
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Jan Beulich 3 years, 2 months ago 
On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
> 
> On 27.09.21 13:00, Jan Beulich wrote:
>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>>         *((u8*) &pdev->bus) = bus;
>>>>>>>         *((u8*) &pdev->devfn) = devfn;
>>>>>>>         pdev->domain = NULL;
>>>>>>> +#ifdef CONFIG_ARM
>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>> +#endif
>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>> obstacles I'm overlooking.
>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>> this is just an architecture specific part of the pdev allocation rather than
>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>>> more natural to me.
>> The bulk of the function is about populating the just allocated struct.
>> There's no arch-specific part of the allocation (so far, leaving aside
>> MSI-X), you only want and arch-specific part of the initialization. I
>> would agree with "alloc" in the name if further allocation was to
>> happen there.
> Hm, then arch_pci_init_pdev sounds more reasonable

Fine with me.

Jan
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 27.09.21 13:26, Jan Beulich wrote:
> On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
>> On 27.09.21 13:00, Jan Beulich wrote:
>>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>>>          *((u8*) &pdev->bus) = bus;
>>>>>>>>          *((u8*) &pdev->devfn) = devfn;
>>>>>>>>          pdev->domain = NULL;
>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>>> +#endif
>>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>>> obstacles I'm overlooking.
>>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>>> this is just an architecture specific part of the pdev allocation rather than
>>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>>>> more natural to me.
>>> The bulk of the function is about populating the just allocated struct.
>>> There's no arch-specific part of the allocation (so far, leaving aside
>>> MSI-X), you only want and arch-specific part of the initialization. I
>>> would agree with "alloc" in the name if further allocation was to
>>> happen there.
>> Hm, then arch_pci_init_pdev sounds more reasonable
> Fine with me.

Do we want this to be void or returning an error code? If error code is needed,

then we would also need a roll-back function, e.g. arch_pci_free_pdev or

arch_pci_release_pdev or arch_pci_fini_pdev or something, so it can be used in

case of error or in free_pdev function.

If so, then what's your preference on the name of that function?

>
> Jan
>
>
Thank you,

Oleksandr
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Jan Beulich 3 years, 2 months ago 
On 28.09.2021 10:09, Oleksandr Andrushchenko wrote:
> 
> On 27.09.21 13:26, Jan Beulich wrote:
>> On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 13:00, Jan Beulich wrote:
>>>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>>>>          *((u8*) &pdev->bus) = bus;
>>>>>>>>>          *((u8*) &pdev->devfn) = devfn;
>>>>>>>>>          pdev->domain = NULL;
>>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>>>> +#endif
>>>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>>>> obstacles I'm overlooking.
>>>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>>>> this is just an architecture specific part of the pdev allocation rather than
>>>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>>>>> more natural to me.
>>>> The bulk of the function is about populating the just allocated struct.
>>>> There's no arch-specific part of the allocation (so far, leaving aside
>>>> MSI-X), you only want and arch-specific part of the initialization. I
>>>> would agree with "alloc" in the name if further allocation was to
>>>> happen there.
>>> Hm, then arch_pci_init_pdev sounds more reasonable
>> Fine with me.
> 
> Do we want this to be void or returning an error code? If error code is needed,
> then we would also need a roll-back function, e.g. arch_pci_free_pdev or
> arch_pci_release_pdev or arch_pci_fini_pdev or something, so it can be used in
> case of error or in free_pdev function.

I'd start with void and make it return an error (and deal with necessary
cleanup) only once a need arises.

Jan
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 28.09.21 11:26, Jan Beulich wrote:
> On 28.09.2021 10:09, Oleksandr Andrushchenko wrote:
>> On 27.09.21 13:26, Jan Beulich wrote:
>>> On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 13:00, Jan Beulich wrote:
>>>>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>>>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>>>>>           *((u8*) &pdev->bus) = bus;
>>>>>>>>>>           *((u8*) &pdev->devfn) = devfn;
>>>>>>>>>>           pdev->domain = NULL;
>>>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>>>>> +#endif
>>>>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>>>>> obstacles I'm overlooking.
>>>>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>>>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>>>>> this is just an architecture specific part of the pdev allocation rather than
>>>>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>>>>>> more natural to me.
>>>>> The bulk of the function is about populating the just allocated struct.
>>>>> There's no arch-specific part of the allocation (so far, leaving aside
>>>>> MSI-X), you only want and arch-specific part of the initialization. I
>>>>> would agree with "alloc" in the name if further allocation was to
>>>>> happen there.
>>>> Hm, then arch_pci_init_pdev sounds more reasonable
>>> Fine with me.
>> Do we want this to be void or returning an error code? If error code is needed,
>> then we would also need a roll-back function, e.g. arch_pci_free_pdev or
>> arch_pci_release_pdev or arch_pci_fini_pdev or something, so it can be used in
>> case of error or in free_pdev function.
> I'd start with void and make it return an error (and deal with necessary
> cleanup) only once a need arises.

Sounds reasonable. For x86 I think we can deal with:

xen/include/xen/pci.h:

#ifdef CONFIG_ARM
void arch_pci_init_pdev(struct pci_dev *pdev);
#else
static inline void arch_pci_init_pdev(struct pci_dev *pdev)
{
     return 0;
}
#endif

>
> Jan
>
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Jan Beulich 3 years, 2 months ago 
On 28.09.2021 10:29, Oleksandr Andrushchenko wrote:
> 
> On 28.09.21 11:26, Jan Beulich wrote:
>> On 28.09.2021 10:09, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 13:26, Jan Beulich wrote:
>>>> On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
>>>>> On 27.09.21 13:00, Jan Beulich wrote:
>>>>>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>>>>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>>>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>>>>>>           *((u8*) &pdev->bus) = bus;
>>>>>>>>>>>           *((u8*) &pdev->devfn) = devfn;
>>>>>>>>>>>           pdev->domain = NULL;
>>>>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>>>>>> +#endif
>>>>>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>>>>>> obstacles I'm overlooking.
>>>>>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>>>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>>>>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>>>>>> this is just an architecture specific part of the pdev allocation rather than
>>>>>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>>>>>>> more natural to me.
>>>>>> The bulk of the function is about populating the just allocated struct.
>>>>>> There's no arch-specific part of the allocation (so far, leaving aside
>>>>>> MSI-X), you only want and arch-specific part of the initialization. I
>>>>>> would agree with "alloc" in the name if further allocation was to
>>>>>> happen there.
>>>>> Hm, then arch_pci_init_pdev sounds more reasonable
>>>> Fine with me.
>>> Do we want this to be void or returning an error code? If error code is needed,
>>> then we would also need a roll-back function, e.g. arch_pci_free_pdev or
>>> arch_pci_release_pdev or arch_pci_fini_pdev or something, so it can be used in
>>> case of error or in free_pdev function.
>> I'd start with void and make it return an error (and deal with necessary
>> cleanup) only once a need arises.
> 
> Sounds reasonable. For x86 I think we can deal with:
> 
> xen/include/xen/pci.h:
> 
> #ifdef CONFIG_ARM
> void arch_pci_init_pdev(struct pci_dev *pdev);
> #else
> static inline void arch_pci_init_pdev(struct pci_dev *pdev)
> {
>      return 0;
> }
> #endif

But that's still #ifdef-ary. We have asm/pci.h.

Jan
Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one
Posted by Oleksandr Andrushchenko 3 years, 2 months ago 
On 28.09.21 11:39, Jan Beulich wrote:
> On 28.09.2021 10:29, Oleksandr Andrushchenko wrote:
>> On 28.09.21 11:26, Jan Beulich wrote:
>>> On 28.09.2021 10:09, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 13:26, Jan Beulich wrote:
>>>>> On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
>>>>>> On 27.09.21 13:00, Jan Beulich wrote:
>>>>>>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>>>>>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>>>>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>>>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>>>>>>>>>>>            *((u8*) &pdev->bus) = bus;
>>>>>>>>>>>>            *((u8*) &pdev->devfn) = devfn;
>>>>>>>>>>>>            pdev->domain = NULL;
>>>>>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>>>>>>> +#endif
>>>>>>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>>>>>>> obstacles I'm overlooking.
>>>>>>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>>>>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>>>>>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>>>>>>> this is just an architecture specific part of the pdev allocation rather than
>>>>>>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev seems
>>>>>>>> more natural to me.
>>>>>>> The bulk of the function is about populating the just allocated struct.
>>>>>>> There's no arch-specific part of the allocation (so far, leaving aside
>>>>>>> MSI-X), you only want and arch-specific part of the initialization. I
>>>>>>> would agree with "alloc" in the name if further allocation was to
>>>>>>> happen there.
>>>>>> Hm, then arch_pci_init_pdev sounds more reasonable
>>>>> Fine with me.
>>>> Do we want this to be void or returning an error code? If error code is needed,
>>>> then we would also need a roll-back function, e.g. arch_pci_free_pdev or
>>>> arch_pci_release_pdev or arch_pci_fini_pdev or something, so it can be used in
>>>> case of error or in free_pdev function.
>>> I'd start with void and make it return an error (and deal with necessary
>>> cleanup) only once a need arises.
>> Sounds reasonable. For x86 I think we can deal with:
>>
>> xen/include/xen/pci.h:
>>
>> #ifdef CONFIG_ARM
>> void arch_pci_init_pdev(struct pci_dev *pdev);
>> #else
>> static inline void arch_pci_init_pdev(struct pci_dev *pdev)
>> {
>>       return 0;
>> }
>> #endif
> But that's still #ifdef-ary. We have asm/pci.h.
Sure, will define it there
>
> Jan
>
>
Thank you,

Oleksandr

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.