Re: [Xen-devel] [PATCH 4 of 4] Linux pvops: xen pci platform device driver

[Bastian Blank <waldi@xxxxxxxxxx>]

On Tue, Mar 02, 2010 at 06:31:10PM +0000, Stefano Stabellini wrote:
> +config XEN_PLATFORM_PCI
> +     bool "xen platform pci device driver"

Case? Why does this need to be built-in?

> +     depends on XEN
> +     select XEN_XENBUS_FRONTEND
> +     help
> +       Necessary to run Xen PV drivers in Xen HVM domains.

A little bit short.

> -     gsv.version = 2;
> +     if (xen_hvm_domain())
> +             gsv.version = 1;
> +     else
> +             gsv.version = 2;
>       rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
>       if (rc == 0)
> -             grant_table_version = 2;
> +             grant_table_version = xen_hvm_domain() ? 1 : 2;

Why is version 1 grant table the default on HVM?

> -             grant_table_version = 1;
> +             grant_table_version = xen_hvm_domain() ? 2 : 1;

But then, this makes no sense for me.

> -static int __devinit gnttab_init(void)
> +int gnttab_init(void)

I miss an export for this function.

> -core_initcall(gnttab_init);
> +static int __devinit __gnttab_init(void)
> +{
> +     if (!xen_domain())
> +             return -ENODEV;

Shouldn't this read xen_pv_domain?

> +/* NB. [aux-]ide-disks options do not unplug IDE CD-ROM drives. */
> +/* NB. aux-ide-disks is equiv to ide-disks except ignores primary master. */
> +static char *dev_unplug;
> +module_param(dev_unplug, charp, 0644);
> +MODULE_PARM_DESC(dev_unplug, "Emulated devices to unplug: "
> +              "[all,][ide-disks,][aux-ide-disks,][nics]\n");

What is this parameter about?

> +#define XEN_IOPORT_BASE 0x10

Why are this constants defined in the driver themself? They only make
sense if there are two components making use of them.

> +#define XEN_IOPORT_PLATFLAGS (XEN_IOPORT_BASE + 0) /* 1 byte access (R/W) */
> +#define XEN_IOPORT_MAGIC     (XEN_IOPORT_BASE + 0) /* 2 byte access (R) */
> +#define XEN_IOPORT_UNPLUG    (XEN_IOPORT_BASE + 0) /* 2 byte access (W) */
> +#define XEN_IOPORT_DRVVER    (XEN_IOPORT_BASE + 0) /* 4 byte access (W) */
> +
> +#define XEN_IOPORT_SYSLOG    (XEN_IOPORT_BASE + 2) /* 1 byte access (W) */
> +#define XEN_IOPORT_PROTOVER  (XEN_IOPORT_BASE + 2) /* 1 byte access (R) */
> +#define XEN_IOPORT_PRODNUM   (XEN_IOPORT_BASE + 2) /* 2 byte access (W) */
> +
> +#define XEN_IOPORT_MAGIC_VAL 0x49d2
> +#define XEN_IOPORT_LINUX_PRODNUM 0xffff /* NB: register a proper one */

What does this "register a proper one" mean?

> +#define XEN_IOPORT_LINUX_DRVVER  ((LINUX_VERSION_CODE << 8) + 0x0)
> +
> +#define UNPLUG_ALL_IDE_DISKS 1
> +#define UNPLUG_ALL_NICS 2
> +#define UNPLUG_AUX_IDE_DISKS 4
> +#define UNPLUG_ALL 7
> +
> +static int check_platform_magic(struct device *dev, long ioaddr, long iolen)
> +{
> +     short magic, unplug = 0;
> +     char protocol, *p, *q, *err;
> +
> +     for (p = dev_unplug; p; p = q) {
> +             q = strchr(dev_unplug, ',');
> +             if (q)
> +                     *q++ = '\0';
> +             if (!strcmp(p, "all"))
> +                     unplug |= UNPLUG_ALL;
> +             else if (!strcmp(p, "ide-disks"))
> +                     unplug |= UNPLUG_ALL_IDE_DISKS;
> +             else if (!strcmp(p, "aux-ide-disks"))
> +                     unplug |= UNPLUG_AUX_IDE_DISKS;
> +             else if (!strcmp(p, "nics"))
> +                     unplug |= UNPLUG_ALL_NICS;
> +             else
> +                     dev_warn(dev, "unrecognised option '%s' "
> +                              "in module parameter 'dev_unplug'\n", p);
> +     }

Usually this is done during the parameter setup.

> +     if (request_mem_region(mmio_addr, mmio_len, DRV_NAME) == NULL) {
> +             printk(KERN_ERR ":MEM I/O resource 0x%lx @ 0x%lx busy\n",
> +                    mmio_addr, mmio_len);
> +             return -EBUSY;

Why is this a sign of busy?

> +     if ((ret = gnttab_init()))
> +             goto out;
> +
> +     if ((ret = xenbus_probe_init()))
> +             goto out;
> +
> + out:
> +     if (ret) {
> +             release_mem_region(mmio_addr, mmio_len);
> +             release_region(ioaddr, iolen);

If xenbus inits, this does not undo the gnttab init?

> +#define XEN_PLATFORM_VENDOR_ID 0x5853
> +#define XEN_PLATFORM_DEVICE_ID 0x0001
> +static struct pci_device_id platform_pci_tbl[] __devinitdata = {
> +     {XEN_PLATFORM_VENDOR_ID, XEN_PLATFORM_DEVICE_ID,
> +      PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +     /* Continue to recognise the old ID for now */
> +     {0xfffd, 0x0101, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},

What is the reasoning for this?

> +static int pci_device_registered;

What is this for?

Bastian

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel