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

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

[Chuck Zmudzinski]

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>
---
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

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
> 

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

[Michael Tokarev]

08.02.2023 05:03, 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>

Has this change been forgotten?  Is it not needed anymore?

Thanks,

/mjt

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

[Chuck Zmudzinski]

On 5/17/2023 2:39 AM, Michael Tokarev wrote:
> 08.02.2023 05:03, 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>
>
> Has this change been forgotten?  Is it not needed anymore?

Short answer:

After 4f67543b ("xen/pt: reserve PCI slot 2 for Intel igd-passthru ") was
applied, I was inclined to think this change is not needed anymore, but
it would not hurt to add this change also, and now I think it might be
more correct to also add this change.

Longer explanation:

I strongly desired that at least one of the patches I proposed to improve
support for Intel IGD passthrough with xen be committed. Since
4f67543b ("xen/pt: reserve PCI slot 2 for Intel igd-passthru ") that fixed
Intel IGD passthrough for the xenfv machine type has been committed,
I reasoned that there is not such a great need to also fix Intel IGD
passthrough for the pc machine type with xen so I did not push hard for
this patch to also be applied.

My requirement was that either the xenfv machine be fixed or the pc
machine be fixed. I did not think it was necessary to fix them both, and
4f67543b ("xen/pt: reserve PCI slot 2 for Intel igd-passthru ") fixed the
xenfv machine. But this patch provides the additional fix for the pc machine,
a fix that is distinct from the fix that has already been committed for the
xenfv machine, and it probably should also be applied so pc and xenfv
machines will work equally well with Intel IGD passthrough.

In other words, it is good to fix at least one of the two broken machines configured
for Intel IGD passthrough and xen, it is better to fix them both. We already fixed
one of them with 4f67543b ("xen/pt: reserve PCI slot 2 for Intel igd-passthru "),
this patch would fix the other one.

If you want to add this change also, let's make sure recent changes to the
xen header files do not require the patch to be rebased before committing
it.

Chuck

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

[Michael Tokarev]

17.05.2023 12:47, Chuck Zmudzinski wrote:
> On 5/17/2023 2:39 AM, Michael Tokarev wrote:
>> 08.02.2023 05:03, Chuck Zmudzinski wrote:...
>>> Fixes: 998250e97661 ("xen, gfx passthrough: register host bridge specific to passthrough")
>>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>>
>> Has this change been forgotten?  Is it not needed anymore?
> 
> Short answer:
> 
> After 4f67543b ("xen/pt: reserve PCI slot 2 for Intel igd-passthru ") was
> applied, I was inclined to think this change is not needed anymore, but
> it would not hurt to add this change also, and now I think it might be
> more correct to also add this change.
...

Well, there were two machines with broken IDG passthrough in xen, now
there's one machine with broken IDG passthrough. Let's fix them all :)
Note this patch is tagged -stable as well.

> If you want to add this change also, let's make sure recent changes to the
> xen header files do not require the patch to be rebased before committing
> it.

It doesn't require rebasing, it looks like, - just built 8.0 and current master
qemu with it applied.  I haven't tried the actual IDG passthrough, though.

It just neeeds to be picked up the usual way as all other qemu changes goes in.

Thanks,

/mjt

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

[Stefano Stabellini]

On Wed, 17 May 2023, Michael Tokarev wrote:
> 17.05.2023 12:47, Chuck Zmudzinski wrote:
> > On 5/17/2023 2:39 AM, Michael Tokarev wrote:
> > > 08.02.2023 05:03, Chuck Zmudzinski wrote:...
> > > > Fixes: 998250e97661 ("xen, gfx passthrough: register host bridge
> > > > specific to passthrough")
> > > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> > > 
> > > Has this change been forgotten?  Is it not needed anymore?
> > 
> > Short answer:
> > 
> > After 4f67543b ("xen/pt: reserve PCI slot 2 for Intel igd-passthru ") was
> > applied, I was inclined to think this change is not needed anymore, but
> > it would not hurt to add this change also, and now I think it might be
> > more correct to also add this change.
> ...
> 
> Well, there were two machines with broken IDG passthrough in xen, now
> there's one machine with broken IDG passthrough. Let's fix them all :)
> Note this patch is tagged -stable as well.
> 
> > If you want to add this change also, let's make sure recent changes to the
> > xen header files do not require the patch to be rebased before committing
> > it.
> 
> It doesn't require rebasing, it looks like, - just built 8.0 and current
> master
> qemu with it applied.  I haven't tried the actual IDG passthrough, though.
> 
> It just neeeds to be picked up the usual way as all other qemu changes goes
> in.

Hi Michal,

I am OK with this patch and acked it. However, I think it also needs an
ack from one if the i386 maintainers, Michal T or Marcel.

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

[Michael Tokarev]

17.05.2023 22:48, Stefano Stabellini :
> On Wed, 17 May 2023, Michael Tokarev wrote:
>> 17.05.2023 12:47, Chuck Zmudzinski wrote:
>>> On 5/17/2023 2:39 AM, Michael Tokarev wrote:
>>>> 08.02.2023 05:03, Chuck Zmudzinski wrote:...
>>>>> Fixes: 998250e97661 ("xen, gfx passthrough: register host bridge
>>>>> specific to passthrough")
>>>>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>>>>
>>>> Has this change been forgotten?  Is it not needed anymore?
...
>> It just neeeds to be picked up the usual way as all other qemu changes goes in.

> I am OK with this patch and acked it. However, I think it also needs an
> ack from one if the i386 maintainers, Michal T or Marcel.

Ping again?  It's been quite a while...
@mst, @marcel?

(Just looking at the stable-todo stuff)

/mjt