From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough Date: Mon, 2 Apr 2012 16:43:14 +0100 Message-ID: <4F79C912.9010504@eu.citrix.com> References: <62b1030a2485536caf99.1333363657@kodo2> <20345.50129.248371.995505@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20345.50129.248371.995505@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 02/04/12 16:20, Ian Jackson wrote: > George Dunlap writes ("[Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"): >> +By default pciback only allows PV guests to write "known safe" values into >> +PCI config space. But many devices require writes to other areas of config >> +space in order to operate properly. This tells the pciback driver to >> +allow all writes to PCI config space for this domain and this device. This >> +option should be enabled with caution, as there may be stability or security >> +implications of doing so. > Is this security warning not overly mealy-mouthed ? Surely it should > be more definite. I'm not sure how we can make it more definite. What's possible (i.e., the security implications) entirely depends on the card; and what's likely (i.e., the stability implications) entirely depends on the card and the driver. Short of giving a short discourse on the vices of various cards PCI config space (which is entirely inappropriate for a man page, IMHO), I'm not sure what more we can say. >> +Changes the default value of 'permissive' for all PCI devices for this >> +VM. This can still be overriden on a per-device basis. See the >> +"pci=" section for more information on the "permissive" flag. > And this should mention it as well I think. I thought it was unnecessary to duplicate, but I can do so if you prefer. > >> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned %d", > Please keep the lines to 75-80 characters at most. Ack. > > I think you should consider breakibg out the sysfs writing function > and refactoring with the very similar code in libxl__device_pci_reset, > rather than introducing yet another clone. I shall consider it. :-) -George