Re: [PATCH V7 10/11] xen/arm: translate virtual PCI bus topology for guests

[Oleksandr <olekstysh@xxxxxxxxx>]

On 26.07.22 18:16, Jan Beulich wrote:

Hello Jan

> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> > --- a/xen/arch/arm/vpci.c
> > +++ b/xen/arch/arm/vpci.c
> > @@ -41,6 +41,16 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> >       /* data is needed to prevent a pointer cast on 32bit */
> >       unsigned long data;
> >   
> > +    /*
> > +     * For the passed through devices we need to map their virtual SBDF
> > +     * to the physical PCI device being passed through.
> > +     */
> > +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
> > +    {
> > +        *r = ~0ul;
> > +        return 1;
> > +    }
> I'm probably simply lacking specific Arm-side knowledge, but it strikes
> me as odd that the need for translation would be dependent upon "bridge".

I am afraid I cannot answer immediately.

I will analyze that question and provide an answer later on.



> > --- a/xen/drivers/vpci/vpci.c
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -158,6 +158,32 @@ static void vpci_remove_virtual_device(const struct 
> > pci_dev *pdev)
> >       }
> >   }
> >   
> > +/*
> > + * Find the physical device which is mapped to the virtual device
> > + * and translate virtual SBDF to the physical one.
> > + */
> > +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
> > +{
> > +    struct pci_dev *pdev;
> const wherever possible please (i.e. likely also for the first function
> parameter).

ok, will do


> > --- a/xen/include/xen/vpci.h
> > +++ b/xen/include/xen/vpci.h
> > @@ -276,6 +276,7 @@ static inline bool __must_check vpci_process_pending(struct 
> > vcpu *v)
> >   /* Notify vPCI that device is assigned/de-assigned to/from guest. */
> >   int vpci_assign_device(struct pci_dev *pdev);
> >   void vpci_deassign_device(struct pci_dev *pdev);
> > +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
> >   #else
> >   static inline int vpci_assign_device(struct pci_dev *pdev)
> >   {
> > @@ -285,6 +286,12 @@ static inline int vpci_assign_device(struct pci_dev *pdev)
> >   static inline void vpci_deassign_device(struct pci_dev *pdev)
> >   {
> >   };
> > +
> > +static inline bool vpci_translate_virtual_device(struct domain *d,
> > +                                                 pci_sbdf_t *sbdf)
> > +{
> > +    return false;
> > +}
> Please don't add stubs which aren't really needed (which, afaict, is the
> case for this one).

I assume, this is needed if HAS_VPCI is present, but 
HAS_VPCI_GUEST_SUPPORT is not. And the author added that stub 
specifically to drop a few "#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT" from 
Arm's code.
Or I really missed something?



> Jan
--
Regards,

Oleksandr Tyshchenko