lkml.org 
[lkml]   [2010]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Xen-devel] [PATCH 4 of 4] Linux pvops: xen pci platform device driver
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


\
 
 \ /
  Last update: 2010-03-03 13:19    [W:0.100 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site