diff mbox series

microvm: turn off io reservations for pcie root ports

Message ID 20220603085920.604323-1-kraxel@redhat.com (mailing list archive)
State New, archived
Headers show
Series microvm: turn off io reservations for pcie root ports | expand

Commit Message

Gerd Hoffmann June 3, 2022, 8:59 a.m. UTC
The pcie host bridge has no io window on microvm,
so io reservations will not work.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/microvm.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Philippe Mathieu-Daudé June 4, 2022, 1:25 p.m. UTC | #1
On 3/6/22 10:59, Gerd Hoffmann wrote:
> The pcie host bridge has no io window on microvm,
> so io reservations will not work.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/i386/microvm.c | 6 ++++++
>   1 file changed, 6 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Sergio Lopez June 6, 2022, 5:08 a.m. UTC | #2
On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> The pcie host bridge has no io window on microvm,
> so io reservations will not work.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/i386/microvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Sergio Lopez <slp@redhat.com>
Michael S. Tsirkin June 8, 2022, 4:06 p.m. UTC | #3
On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> The pcie host bridge has no io window on microvm,
> so io reservations will not work.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I don't much like overriding user like this. We end up users
setting it to silly values and then if we do want to
support this things just break. Thoughts?

> ---
>  hw/i386/microvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 4b3b1dd262f1..f01d972f5d28 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -757,6 +757,12 @@ static void microvm_class_init(ObjectClass *oc, void *data)
>          "Set off to disable adding virtio-mmio devices to the kernel cmdline");
>  
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> +
> +    /*
> +     * pcie host bridge (gpex) on microvm has no io address window,
> +     * so reserving io space is not going to work.  Turn it off.
> +     */
> +    object_register_sugar_prop("pcie-root-port", "io-reserve", "0", true);
>  }
>  
>  static const TypeInfo microvm_machine_info = {
> -- 
> 2.36.1
Gerd Hoffmann June 9, 2022, 7:28 a.m. UTC | #4
On Wed, Jun 08, 2022 at 12:06:17PM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> > The pcie host bridge has no io window on microvm,
> > so io reservations will not work.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> I don't much like overriding user like this. We end up users
> setting it to silly values and then if we do want to
> support this things just break. Thoughts?

Well, it just looked like the simplest way to tell the firmware that
io reservations are pointless.  Do you have a better idea?

take care,
  Gerd
Michael S. Tsirkin June 27, 2022, 10:37 p.m. UTC | #5
On Thu, Jun 09, 2022 at 09:28:38AM +0200, Gerd Hoffmann wrote:
> On Wed, Jun 08, 2022 at 12:06:17PM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> > > The pcie host bridge has no io window on microvm,
> > > so io reservations will not work.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > I don't much like overriding user like this. We end up users
> > setting it to silly values and then if we do want to
> > support this things just break. Thoughts?
> 
> Well, it just looked like the simplest way to tell the firmware that
> io reservations are pointless.  Do you have a better idea?
> 
> take care,
>   Gerd

Fail if user supplies values we can't support.
Gerd Hoffmann June 29, 2022, 7:10 a.m. UTC | #6
On Mon, Jun 27, 2022 at 06:37:50PM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 09, 2022 at 09:28:38AM +0200, Gerd Hoffmann wrote:
> > On Wed, Jun 08, 2022 at 12:06:17PM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> > > > The pcie host bridge has no io window on microvm,
> > > > so io reservations will not work.
> > > > 
> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > 
> > > I don't much like overriding user like this. We end up users
> > > setting it to silly values and then if we do want to
> > > support this things just break. Thoughts?
> > 
> > Well, it just looked like the simplest way to tell the firmware that
> > io reservations are pointless.  Do you have a better idea?
> > 
> > take care,
> >   Gerd
> 
> Fail if user supplies values we can't support.

Well, it is the *default* value which doesn't work on microvm.

take care,
  Gerd
Michael S. Tsirkin June 29, 2022, 7:16 a.m. UTC | #7
On Wed, Jun 29, 2022 at 09:10:23AM +0200, Gerd Hoffmann wrote:
> On Mon, Jun 27, 2022 at 06:37:50PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 09, 2022 at 09:28:38AM +0200, Gerd Hoffmann wrote:
> > > On Wed, Jun 08, 2022 at 12:06:17PM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> > > > > The pcie host bridge has no io window on microvm,
> > > > > so io reservations will not work.
> > > > > 
> > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > > 
> > > > I don't much like overriding user like this. We end up users
> > > > setting it to silly values and then if we do want to
> > > > support this things just break. Thoughts?
> > > 
> > > Well, it just looked like the simplest way to tell the firmware that
> > > io reservations are pointless.  Do you have a better idea?
> > > 
> > > take care,
> > >   Gerd
> > 
> > Fail if user supplies values we can't support.
> 
> Well, it is the *default* value which doesn't work on microvm.
> 
> take care,
>   Gerd

Changing defaults is ok of course. Let's just not override the user.
Gerd Hoffmann June 29, 2022, 12:20 p.m. UTC | #8
On Wed, Jun 29, 2022 at 03:16:17AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jun 29, 2022 at 09:10:23AM +0200, Gerd Hoffmann wrote:
> > On Mon, Jun 27, 2022 at 06:37:50PM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jun 09, 2022 at 09:28:38AM +0200, Gerd Hoffmann wrote:
> > > > On Wed, Jun 08, 2022 at 12:06:17PM -0400, Michael S. Tsirkin wrote:
> > > > > On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> > > > > > The pcie host bridge has no io window on microvm,
> > > > > > so io reservations will not work.
> > > > > > 
> > > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > > > 
> > > > > I don't much like overriding user like this. We end up users
> > > > > setting it to silly values and then if we do want to
> > > > > support this things just break. Thoughts?
> > > > 
> > > > Well, it just looked like the simplest way to tell the firmware that
> > > > io reservations are pointless.  Do you have a better idea?
> > > > 
> > > > take care,
> > > >   Gerd
> > > 
> > > Fail if user supplies values we can't support.
> > 
> > Well, it is the *default* value which doesn't work on microvm.
> > 
> > take care,
> >   Gerd
> 
> Changing defaults is ok of course. Let's just not override the user.

Ok, so I could use a compat property instead and change the default
for microvm that way.  That would allow the user set any value it
wants.

I still don't see the point though.  There is only a single value which
makes sense on microvm.  Which is zero.  The only effect the user could
archive is make the firmware throwing errors ...

take care,
  Gerd
Michael S. Tsirkin June 29, 2022, 2:04 p.m. UTC | #9
On Wed, Jun 29, 2022 at 02:20:50PM +0200, Gerd Hoffmann wrote:
> On Wed, Jun 29, 2022 at 03:16:17AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jun 29, 2022 at 09:10:23AM +0200, Gerd Hoffmann wrote:
> > > On Mon, Jun 27, 2022 at 06:37:50PM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, Jun 09, 2022 at 09:28:38AM +0200, Gerd Hoffmann wrote:
> > > > > On Wed, Jun 08, 2022 at 12:06:17PM -0400, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jun 03, 2022 at 10:59:20AM +0200, Gerd Hoffmann wrote:
> > > > > > > The pcie host bridge has no io window on microvm,
> > > > > > > so io reservations will not work.
> > > > > > > 
> > > > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > > > > 
> > > > > > I don't much like overriding user like this. We end up users
> > > > > > setting it to silly values and then if we do want to
> > > > > > support this things just break. Thoughts?
> > > > > 
> > > > > Well, it just looked like the simplest way to tell the firmware that
> > > > > io reservations are pointless.  Do you have a better idea?
> > > > > 
> > > > > take care,
> > > > >   Gerd
> > > > 
> > > > Fail if user supplies values we can't support.
> > > 
> > > Well, it is the *default* value which doesn't work on microvm.
> > > 
> > > take care,
> > >   Gerd
> > 
> > Changing defaults is ok of course. Let's just not override the user.
> 
> Ok, so I could use a compat property instead and change the default
> for microvm that way.  That would allow the user set any value it
> wants.

And if you like check the value and fail init if not 0.

> I still don't see the point though.  There is only a single value which
> makes sense on microvm.  Which is zero.  The only effect the user could
> archive is make the firmware throwing errors ...
> 
> take care,
>   Gerd

My concern is simple: if right now we override the value then
some users might set it to != 0 by mistake. Then if down the road
we want to intepret != 0 in some way, these setups
will start failing. I don't claim to see a use-case why we'd want to
but it's hard to predict the future with certainty.
diff mbox series

Patch

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 4b3b1dd262f1..f01d972f5d28 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -757,6 +757,12 @@  static void microvm_class_init(ObjectClass *oc, void *data)
         "Set off to disable adding virtio-mmio devices to the kernel cmdline");
 
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+
+    /*
+     * pcie host bridge (gpex) on microvm has no io address window,
+     * so reserving io space is not going to work.  Turn it off.
+     */
+    object_register_sugar_prop("pcie-root-port", "io-reserve", "0", true);
 }
 
 static const TypeInfo microvm_machine_info = {