Re: PCI Pass-through in Xen ARM - Draft 2.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Sun, Jul 05, 2015 at 11:25:25AM +0530, Manish Jaggi wrote:
> 
> 
> On Monday 29 June 2015 04:01 PM, Julien Grall wrote:
> >Hi Manish,
> >
> >On 28/06/15 19:38, Manish Jaggi wrote:
> >>4.1 Holes in guest memory space
> >>----------------------------
> >>Holes are added in the guest memory space for mapping pci device's BAR
> >>regions.
> >>These are defined in arch-arm.h
> >>
> >>/* For 32bit */
> >>GUEST_MMIO_HOLE0_BASE, GUEST_MMIO_HOLE0_SIZE
> >>/* For 64bit */
> >>GUEST_MMIO_HOLE1_BASE , GUEST_MMIO_HOLE1_SIZE
> >The memory layout for 32bit and 64bit are exactly the same. Why do you
> >need to differ here?
> I think Ian has already replied. I will change the name of macro
> >>4.2 New entries in xenstore for device BARs
> >>--------------------------------------------
> >>toolkit also updates the xenstore information for the device
> >>(virtualbar:physical bar).
> >>This information is read by xenpciback and returned to the pcifront
> >>driver configuration
> >>space accesses.
> >Can you details what do you plan to put in xenstore and how?
> It is implementation . But I plan to put under domU / device / heirarchy
> >What about the expansion ROM?
> Do you want to put some restriction on not using expansion ROM as a
> passthrough device.
> >
> >>4.3 Hypercall for bdf mapping notification to xen
> >>-----------------------------------------------
> >>#define PHYSDEVOP_map_sbdf              43
> >>typedef struct {
> >>     u32 s;
> >>     u8 b;
> >>     u8 df;
> >>     u16 res;
> >>} sbdf_t;
> >>struct physdev_map_sbdf {
> >>     int domain_id;
> >>     sbdf_t    sbdf;
> >>     sbdf_t    gsbdf;
> >>};
> >>
> >>Each domain has a pdev list, which contains the list of all pci devices.
> >>The
> >>pdev structure already has a sbdf information. The arch_pci_dev is
> >>updated to
> >>contain the gsbdf information. (gs- guest segment id)
> >>
> >>Whenever there is trap from guest or an interrupt has to be injected,
> >>the pdev
> >>list is iterated to find the gsbdf.
> >Can you give more background for this section? i.e:
> >	- Why do you need this?
> >	- How xen will translate the gbdf to a vDeviceID?
> In the context of the hypercall processing.
> >	- Who will call this hypercall?
> >	- Why not setting the gsbdf when the device is assigned?
> Can the maintainer of the pciback suggest an alternate.
> The answer to your question is that I have only found a place to issue the
> hypercall where
> all the information can be located is the function
> __xen_pcibk_add_pci_dev
> 
> 
> drivers/xen/xen-pciback/vpci.c
> ----
> unlock:
> ...
>                 kfree(dev_entry);
> 
> +       /*Issue Hypercall here */
> +#ifdef CONFIG_ARM64
> +       map_sbdf.domain_id = pdev->xdev->otherend_id;
> +       map_sbdf.sbdf_s = dev->bus->domain_nr;
> +       map_sbdf.sbdf_b = dev->bus->number;
> +       map_sbdf.sbdf_d = dev->devfn>>3;
> +       map_sbdf.sbdf_f = dev->devfn & 0x7;
> +       map_sbdf.gsbdf_s = 0;
> +       map_sbdf.gsbdf_b = 0;
> +       map_sbdf.gsbdf_d = slot;
> +       map_sbdf.gsbdf_f = dev->devfn & 0x7;
> +       pr_info("## sbdf = %d:%d:%d.%d g_sbdf %d:%d:%d.%d \
> +                       domain_id=%d ##\r\n",
> +                       map_sbdf.sbdf_s,
> +                       map_sbdf.sbdf_b,
> +                       map_sbdf.sbdf_d,
> +                       map_sbdf.sbdf_f,
> +                       map_sbdf.gsbdf_s,
> +                       map_sbdf.gsbdf_b,
> +                       map_sbdf.gsbdf_d,
> +                       map_sbdf.gsbdf_f,
> +                       map_sbdf.domain_id);
> +
> +       err = HYPERVISOR_physdev_op(PHYSDEVOP_map_sbdf, &map_sbdf);
> +       if (err)
> +               printk(KERN_ERR " Xen Error PHYSDEVOP_map_sbdf");
> +#endif

You probably also need the inverse - that is when xen_pcibk_release_pci_dev
is called.

I would suggest you instead add in  drivers/xen/xen-pciback/pciback.h
for xen_pcibk_add_pci_dev and xen_pcibk_release_pci_dev an call
to the arch specific code for this hypercall. On x86 it would be a nop
while for ARM it would the above.

> ---
> 
> >Regards,
> >
>