On 14.03.2022 04:41, Chuck Zmudzinski wrote:
> Fixes: abfb006f1ff4 (tools/libxl: explicitly grant access to needed
> I/O-memory ranges)
> Fixes: 0561e1f01e87 (xen/common: do not implicitly permit access to mapped
> I/O memory)
> Backport: 4.12+
Just fyi: This is fine to have as a tag, but it wouldn't be backported
farther than to 4.15.
Apart from this largely some style issues (see below), but please
realize that I'm not a libxl maintainer and hence I may not have good
enough knowledge of, in particular, potential unwritten conventions.
> @@ -610,6 +612,45 @@ out:
> return ret;
> }
>
> +static uint32_t sysfs_dev_get_igd_opregion(libxl__gc *gc,
> + libxl_device_pci *pci)
> +{
> + char *pci_device_config_path =
> + GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/config",
> + pci->domain, pci->bus, pci->dev, pci->func);
> + size_t read_items;
> + uint32_t igd_opregion;
> + uint32_t error = 0xffffffff;
I think this constant wants to gain a #define, to be able to correlate
the use sites. I'm also not sure of the value - in principle the
register can hold this value, but of course then it won't be 3 pages.
Maybe the error check further down should be to see whether adding 2
to the value would overflow in 32 bits? (In that case a #define may
not be needed anymore, as there wouldn't be multiple instances of the
constant in the code.)
> +
> + FILE *f = fopen(pci_device_config_path, "r");
> + if (!f) {
While libxl has some special style rules, I think it still wants a
blank line between declaration(s) and statement(s), just like we
expect elsewhere. Effectively you want to simply move the blank line
you have one line down.
> @@ -2531,6 +2572,37 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc,
> const uint32_t domid,
> domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> return ret;
> }
> +
> + /* If this is an Intel IGD, allow access to the IGD opregion */
> + if (!libxl__is_igd_vga_passthru(gc, d_config)) return 0;
Despite the provision for "return" or alike to go on the same line
as an error code check, I don't think this is okay here. It would be
if, as iirc generally expected in libxl, you latched the function
return value into a local variable named "rc" (I think).
> + uint32_t igd_opregion = sysfs_dev_get_igd_opregion(gc, pci);
> + uint32_t error = 0xffffffff;
Please don't mix declarations and statements. I also don't think
"error" is really necessary as a local variable, but with the change
suggested above it might disappear anyway.
> + if (igd_opregion == error) break;
Like above I'm not sure this is okay to all live on one line. I also
think it would be nice if you used "return 0" or "break" consistently.
Of course a related question is whether failure here should actually
be reported to the caller.
> + vga_iomem_start = ( (uint64_t) igd_opregion ) >> XC_PAGE_SHIFT;
There's no need for a cast here, as you're right-shifting. Also
(just fyi) there would have been three to many spaces here. I'm
additionally not certain whether re-using a variable for a purpose
not matching its name is deemed acceptable by libxl maintainers.
> + ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid,
> + vga_iomem_start,
> + IGD_OPREGION_PAGES, 1);
> + if (ret < 0) {
> + LOGED(ERROR, domid,
> + "failed to give stubdom%d access to iomem range "
> + "%"PRIx64"-%"PRIx64" for IGD passthru",
> + stubdom_domid, vga_iomem_start, (vga_iomem_start +
> + IGD_OPREGION_PAGES - 1));
> + return ret;
> + }
I have to admit that I find it odd that this is done unconditionally,
but I notice the same is done in pre-existing code. I would have
expected this to happen only when there actually is a device model
stub domain.
Jan
> + ret = xc_domain_iomem_permission(CTX->xch, domid,
> + vga_iomem_start,
> + IGD_OPREGION_PAGES, 1);
> + if (ret < 0) {
> + LOGED(ERROR, domid,
> + "failed to give dom%d access to iomem range "
> + "%"PRIx64"-%"PRIx64" for IGD passthru",
> + domid, vga_iomem_start, (vga_iomem_start +
> + IGD_OPREGION_PAGES - 1));
> + return ret;
> + }
> break;
> }
>
