| Message ID | 20210923125438.234162-6-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 2 | expand |
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 >
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 >>
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
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
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
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
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
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 >
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
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
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
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 >
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
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
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 )