Re: [PATCH] xen/pt: fix igd passthrough for pc machine with xen accelerator

[Stefano Stabellini]

On Tue, 7 Feb 2023, Chuck Zmudzinski wrote:
> Commit 998250e97661 ("xen, gfx passthrough: register host bridge specific
> to passthrough") uses the igd-passthrough-i440FX pci host device with
> the xenfv machine type and igd-passthru=on, but using it for the pc
> machine type, xen accelerator, and igd-passtru=on was omitted from that
> commit.
> 
> The igd-passthru-i440FX pci host device is also needed for guests
> configured with the pc machine type, the xen accelerator, and
> igd-passthru=on. Specifically, tests show that not using the igd-specific
> pci host device with the Intel igd passed through to the guest results
> in slower startup performance and reduced resolution of the display
> during startup. This patch fixes this issue.
> 
> To simplify the logic that is needed to support both the --enable-xen
> and the --disable-xen configure options, introduce the boolean symbol
> pc_xen_igd_gfx_pt_enabled() whose value is set appropriately in the
> sysemu/xen.h header file as the test to determine whether or not
> to use the igd-passthrough-i440FX pci host device instead of the
> normal i440FX pci host device.
> 
> Fixes: 998250e97661 ("xen, gfx passthrough: register host bridge specific to 
> passthrough")
> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>

I think this is OK

Acked-by: Stefano Stabellini <sstabellini@kernel.org>



> ---
> This patch is intended to replace or complement a recently proposed
> patch that modifies slot_reserved_mask for the xenfv machine with
> igd-passthru=on in order to fix the problem of Qemu not reserving slot 2
> for the Intel IGD for the xenfv machine type. This patch provides a
> simple way to improve Qemu support for the Intel IGD with the xen
> accelerator without needing to change how slot_reserved_mask functions.
> 
> For reference, the latest version of the patch to fix the xenfv machine
> using slot_reserved_mask is at:
> 
> https://lore.kernel.org/qemu-devel/b1b4a21fe9a600b1322742dda55a40e9961daa57.1674346505.git.brchuckz@aol.com/
> 
> Reason for introducing the new boolean pc_xen_igd_gfx_pt_enabled():
> 
> It is also possible to use xen_igd_gfx_pt_enabled() directly to check
> if the igd-passthru-i440FX pci host device is needed in this patch,
> but in that case it would be necessary to implement it in
> accel/stubs/xen-stub.c like this:
> 
> bool xen_igd_gfx_pt_enabled(void)
> {
>     return false;
> }
> 
> to cover the case when Qemu is configured with --disable-xen. I thought
> it was simpler to introduce the same boolean prefixed with pc_ and
> set it to 0 when --disable-xen is the configure option, and that explains
> why the proposed patch introduces pc_xen_igd_gfx_pt_enabled() instead of
> using xen_igd_gfx_pt_enabled() directly.
> 
> Another reason to use pc_xen_igd_gfx_pt_enabled() is to distinguish it
> from xen_igd_gfx_pt_enabled() in hw/i386/pc_piix.c, because the use of
> xen_igd_gfx_pt_enabled() is guarded by CONFIG_XEN but this patch needs
> to place the boolean in a position that is not guarded by CONFIG_XEN.
> This approach will simplify any future effort to move the code in
> pc_piix.c that is not guarded by CONFIG_XEN to a xen-specific file.
> 
>  hw/i386/pc_piix.c    | 2 ++
>  include/sysemu/xen.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index df64dd8dcc..fd5b9ae1eb 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -433,6 +433,8 @@ static void pc_xen_hvm_init(MachineState *machine)
>              compat(machine); \
>          } \
>          pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, \
> +                 pc_xen_igd_gfx_pt_enabled() ? \
> +                 TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : \
>                   TYPE_I440FX_PCI_DEVICE); \
>      } \
>      DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn)
> diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
> index 0ca25697e4..99ae41e619 100644
> --- a/include/sysemu/xen.h
> +++ b/include/sysemu/xen.h
> @@ -23,6 +23,7 @@
>  extern bool xen_allowed;
>  
>  #define xen_enabled()           (xen_allowed)
> +#define pc_xen_igd_gfx_pt_enabled()    xen_igd_gfx_pt_enabled()
>  
>  #ifndef CONFIG_USER_ONLY
>  void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
> @@ -33,6 +34,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>  #else /* !CONFIG_XEN_IS_POSSIBLE */
>  
>  #define xen_enabled() 0
> +#define pc_xen_igd_gfx_pt_enabled() 0
>  #ifndef CONFIG_USER_ONLY
>  static inline void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t 
> length)
>  {
> -- 
> 2.39.1
>