Re: [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain

["Roger Pau Monné" <roger.pau@citrix.com>]

On Sun, Jun 14, 2020 at 10:12:03PM +0000, Grzegorz Uriasz wrote:
> When passing through a IGD VGA device qemu needs to copy the host VBIOS
> to the target domain. Right now the current implementation on the qemu side
> is one big undefined behavior as described in my qemu patchset here:
> https://patchew.org/QEMU/20200428062847.7764-1-gorbak25@gmail.com/

I think it would be good to elaborate a bit more here on the issue,
that link could go away and we still need to keep the reasoning behind
this change in the Xen changelog IMO.

> This patch is tied to the linked patchset for qemu but fortunately
> this patch still works without the qemu part merged. When the qemu part
> gets merged then qemu will access the VBIOS using /dev/mem - this is
> required as currently the linux kernel forbids accessing this memory
> region when the VBIOS is corrupted - which will always be the case as
> described in the linked patchset. When qemu is running inside a linux
> based stubdomain then the stubdomain does not have access to the VBIOS.
> This patch maps the VBIOS to the stubdomain so qemu with my fixes may
> create a shadow copy for the target domain.
> 
> Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>
> ---
>  tools/libxl/libxl_pci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 48b1d8073b..9b9564dd73 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -2445,6 +2445,8 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom
>      int ret, i;
>      uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
>      uint64_t vga_iomem_npages = 0x20;
> +    uint64_t vga_vbios_start = 0xc0000 >> XC_PAGE_SHIFT;
> +    uint64_t vga_vbios_npages = 0x20;

Is this always 32 pages? Some claim [0] it's 32KiB (so 8 pages).
Wouldn't it be safer to fetch the size from sysfs or some other place
if possible?

>      uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid);
>      uint64_t vga_ioport_start[] = {0x3B0, 0x3C0};
>      uint64_t vga_ioport_size[] = {0xC, 0x20};
> @@ -2460,6 +2462,7 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom
>                vga_iomem_start, (vga_iomem_start + (vga_iomem_npages << XC_PAGE_SHIFT) - 1));
>          return ret;
>      }
> +

Stray change?

>      ret = xc_domain_iomem_permission(CTX->xch, domid,
>                                       vga_iomem_start, vga_iomem_npages, 1);
>      if (ret < 0) {
> @@ -2470,6 +2473,13 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom
>          return ret;
>      }
>  
> +    /* VGA ROM */
> +    ret = xc_domain_memory_mapping(CTX->xch, stubdom_domid, vga_vbios_start, vga_vbios_start, vga_vbios_npages, DPCI_ADD_MAPPING);

Line is too long, please limit lines to 75 characters (see
libxl/CODING_STYLE).

> +    if (ret < 0) {
> +        LOGED(ERROR, domid, "failed to map VBIOS to stubdom%d", stubdom_domid);

AFAICT you have to use LOGEVD here, since the errno value will be
returned in 'ret'. Ie:

if (ret < 0) {
    LOGED(ERROR, ret, domid, "failed to map VBIOS to stubdom%d",
          stubdom_domid);
    return ERROR_FAIL;
}

Thanks, Roger.

[0] https://wiki.osdev.org/Memory_Map_(x86)#Overview