| Message ID | a304213d26506b066021f803c39b87f6a262ed86.1675820085.git.brchuckz@aol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/pt: fix igd passthrough for pc machine with xen accelerator | expand |
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 >
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
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
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
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.
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
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) {
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(+)