diff mbox series

[v5,7/8] xen/arm: introduce nr_spis

Message ID 20190925184924.21691-7-sstabellini@kernel.org (mailing list archive)
State Superseded
Headers show
Series [v5,1/8] xen/arm: introduce handle_device_interrupts | expand

Commit Message

Stefano Stabellini Sept. 25, 2019, 6:49 p.m. UTC
We don't have a clear way to know how many virtual SPIs we need for the
dom0-less domains. Introduce a new option under xen,domain to specify
the number of SPIs to allocate for a domain.

The property is optional. When absent, we'll use the physical number of
GIC lines for dom0-less domains, just like for dom0.

Remove the old setting of nr_spis based on the presence of the vpl011.

The implication of this change is that without nr_spis dom0less domains
get the same amount of SPI allocated as dom0, regardless of how many
physical devices they have assigned, and regardless of whether they have
a virtual pl011 (which also needs an emulated SPI). For instance, we
could end up exposing 256 SPIs for each dom0less domain without a
nr_spis property. If we have 4 dom0less domains without nr_spis, it
would result in 80K of additional memory being used.

When nr_spis is present, the domain gets exactly nr_spis allocated SPIs.
If the number is too low, it might not be enough for the devices
assigned it to it. If the number is less than GUEST_VPL011_SPI, the
virtual pl011 won't work.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
Changes in v5:
- improve commit message
- allocate enough SPIs for vpl011

Changes in v4:
- improve commit message

Changes in v3:
- improve commit message
- introduce nr_spis
---
 xen/arch/arm/domain_build.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Julien Grall Sept. 25, 2019, 9:23 p.m. UTC | #1
Hi,

On 25/09/2019 19:49, Stefano Stabellini wrote:
> We don't have a clear way to know how many virtual SPIs we need for the
> dom0-less domains. Introduce a new option under xen,domain to specify
> the number of SPIs to allocate for a domain.
> 
> The property is optional. When absent, we'll use the physical number of
> GIC lines for dom0-less domains, just like for dom0.

Based on the code below, this is not correct when using vpl011.

> 
> Remove the old setting of nr_spis based on the presence of the vpl011.
> 
> The implication of this change is that without nr_spis dom0less domains
> get the same amount of SPI allocated as dom0, regardless of how many
> physical devices they have assigned, and regardless of whether they have
> a virtual pl011 (which also needs an emulated SPI). For instance, we
> could end up exposing 256 SPIs for each dom0less domain without a
> nr_spis property. If we have 4 dom0less domains without nr_spis, it
> would result in 80K of additional memory being used.

I don't understand what you are trying to imply with your example. Ok, 
this tell you how much memory you are going to waste... but this does 
still not explain why the nr_spis are increased in the default case.

> 
> When nr_spis is present, the domain gets exactly nr_spis allocated SPIs.
> If the number is too low, it might not be enough for the devices
> assigned it to it. If the number is less than GUEST_VPL011_SPI, the
> virtual pl011 won't work.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> Changes in v5:
> - improve commit message
> - allocate enough SPIs for vpl011
> 
> Changes in v4:
> - improve commit message
> 
> Changes in v3:
> - improve commit message
> - introduce nr_spis
> ---
>   xen/arch/arm/domain_build.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 414893bc24..bf4d960eb5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2345,7 +2345,6 @@ void __init create_domUs(void)
>           struct domain *d;
>           struct xen_domctl_createdomain d_cfg = {
>               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> -            .arch.nr_spis = 0,
>               .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>                        XEN_DOMCTL_CDF_iommu,
>               .max_evtchn_port = -1,
> @@ -2356,13 +2355,23 @@ void __init create_domUs(void)
>           if ( !dt_device_is_compatible(node, "xen,domain") )
>               continue;
>   
> -        if ( dt_property_read_bool(node, "vpl011") )
> -            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
> -
>           if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
>               panic("Missing property 'cpus' for domain %s\n",
>                     dt_node_name(node));
>   
> +        if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> +        {
> +            d_cfg.arch.nr_spis = gic_number_lines() - 32;
> +
> +            /*
> +             * vpl011 uses one emulated SPI. If vpl011 is requested, make
> +             * sure that we allocate enough SPIs for it.
> +             */
> +            if ( dt_property_read_bool(node, "vpl011") )
> +                d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> +                                         GUEST_VPL011_SPI - 32 + 1);
> +        }
> +
>           d = domain_create(++max_init_domid, &d_cfg, false);
>           if ( IS_ERR(d) )
>               panic("Error creating domain %s\n", dt_node_name(node));
> 

Cheers,
Stefano Stabellini Sept. 26, 2019, 1:25 a.m. UTC | #2
On Wed, 25 Sep 2019, Julien Grall wrote:
> Hi,
> 
> On 25/09/2019 19:49, Stefano Stabellini wrote:
> > We don't have a clear way to know how many virtual SPIs we need for the
> > dom0-less domains. Introduce a new option under xen,domain to specify
> > the number of SPIs to allocate for a domain.
> > 
> > The property is optional. When absent, we'll use the physical number of
> > GIC lines for dom0-less domains, just like for dom0.
> 
> Based on the code below, this is not correct when using vpl011.

I'll write:

The property is optional. When absent, we'll use the physical number of
GIC lines for dom0-less domains, or GUEST_VPL011_SPI+1 if vpl011 is
requested, whichever is greater.


> > 
> > Remove the old setting of nr_spis based on the presence of the vpl011.
> > 
> > The implication of this change is that without nr_spis dom0less domains
> > get the same amount of SPI allocated as dom0, regardless of how many
> > physical devices they have assigned, and regardless of whether they have
> > a virtual pl011 (which also needs an emulated SPI). For instance, we
> > could end up exposing 256 SPIs for each dom0less domain without a
> > nr_spis property. If we have 4 dom0less domains without nr_spis, it
> > would result in 80K of additional memory being used.
> 
> I don't understand what you are trying to imply with your example. Ok, 
> this tell you how much memory you are going to waste... but this does 
> still not explain why the nr_spis are increased in the default case.

I misunderstood what you wanted me to add to the commit message. I'll
remove the example and instead write:

The implication of this change is that without nr_spis dom0less domains
get the same amount of SPI allocated as dom0, regardless of how many
physical devices they have assigned, and regardless of whether they have
a virtual pl011 (which also needs an emulated SPI). This is done because
the SPIs allocation needs to be done before parsing any passthrough
information, so we have to account for any potential physical SPI
assigned to the domain.


Is this better?



> > When nr_spis is present, the domain gets exactly nr_spis allocated SPIs.
> > If the number is too low, it might not be enough for the devices
> > assigned it to it. If the number is less than GUEST_VPL011_SPI, the
> > virtual pl011 won't work.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > ---
> > Changes in v5:
> > - improve commit message
> > - allocate enough SPIs for vpl011
> > 
> > Changes in v4:
> > - improve commit message
> > 
> > Changes in v3:
> > - improve commit message
> > - introduce nr_spis
> > ---
> >   xen/arch/arm/domain_build.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 414893bc24..bf4d960eb5 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2345,7 +2345,6 @@ void __init create_domUs(void)
> >           struct domain *d;
> >           struct xen_domctl_createdomain d_cfg = {
> >               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> > -            .arch.nr_spis = 0,
> >               .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> >                        XEN_DOMCTL_CDF_iommu,
> >               .max_evtchn_port = -1,
> > @@ -2356,13 +2355,23 @@ void __init create_domUs(void)
> >           if ( !dt_device_is_compatible(node, "xen,domain") )
> >               continue;
> >   
> > -        if ( dt_property_read_bool(node, "vpl011") )
> > -            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
> > -
> >           if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
> >               panic("Missing property 'cpus' for domain %s\n",
> >                     dt_node_name(node));
> >   
> > +        if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> > +        {
> > +            d_cfg.arch.nr_spis = gic_number_lines() - 32;
> > +
> > +            /*
> > +             * vpl011 uses one emulated SPI. If vpl011 is requested, make
> > +             * sure that we allocate enough SPIs for it.
> > +             */
> > +            if ( dt_property_read_bool(node, "vpl011") )
> > +                d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> > +                                         GUEST_VPL011_SPI - 32 + 1);
> > +        }
> > +
> >           d = domain_create(++max_init_domid, &d_cfg, false);
> >           if ( IS_ERR(d) )
> >               panic("Error creating domain %s\n", dt_node_name(node));
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
>
Julien Grall Sept. 26, 2019, 2:11 p.m. UTC | #3
Hi Stefano,

On 9/26/19 2:25 AM, Stefano Stabellini wrote:
> On Wed, 25 Sep 2019, Julien Grall wrote:
>> Hi,
>>
>> On 25/09/2019 19:49, Stefano Stabellini wrote:
>>> We don't have a clear way to know how many virtual SPIs we need for the
>>> dom0-less domains. Introduce a new option under xen,domain to specify
>>> the number of SPIs to allocate for a domain.
>>>
>>> The property is optional. When absent, we'll use the physical number of
>>> GIC lines for dom0-less domains, just like for dom0.
>>
>> Based on the code below, this is not correct when using vpl011.
> 
> I'll write:
> 
> The property is optional. When absent, we'll use the physical number of
> GIC lines for dom0-less domains, or GUEST_VPL011_SPI+1 if vpl011 is
> requested, whichever is greater.

Sounds good to me.

> 
> 
>>>
>>> Remove the old setting of nr_spis based on the presence of the vpl011.
>>>
>>> The implication of this change is that without nr_spis dom0less domains
>>> get the same amount of SPI allocated as dom0, regardless of how many
>>> physical devices they have assigned, and regardless of whether they have
>>> a virtual pl011 (which also needs an emulated SPI). For instance, we
>>> could end up exposing 256 SPIs for each dom0less domain without a
>>> nr_spis property. If we have 4 dom0less domains without nr_spis, it
>>> would result in 80K of additional memory being used.
>>
>> I don't understand what you are trying to imply with your example. Ok,
>> this tell you how much memory you are going to waste... but this does
>> still not explain why the nr_spis are increased in the default case.
> 
> I misunderstood what you wanted me to add to the commit message.

Sorry for the confusion, my main point is you can't really say this is 
low footprint as this is very subjective. Personally, I feel it is a lot 
because if you take the example, this is roughly 8% of the current size 
of Xen (in default config).

> I'll remove the example and instead write:
> 
> The implication of this change is that without nr_spis dom0less domains
> get the same amount of SPI allocated as dom0, regardless of how many
> physical devices they have assigned, and regardless of whether they have
> a virtual pl011 (which also needs an emulated SPI). This is done because
> the SPIs allocation needs to be done before parsing any passthrough
> information, so we have to account for any potential physical SPI
> assigned to the domain.
> 
> 
> Is this better?

Yes, thank you.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 414893bc24..bf4d960eb5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2345,7 +2345,6 @@  void __init create_domUs(void)
         struct domain *d;
         struct xen_domctl_createdomain d_cfg = {
             .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
-            .arch.nr_spis = 0,
             .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
                      XEN_DOMCTL_CDF_iommu,
             .max_evtchn_port = -1,
@@ -2356,13 +2355,23 @@  void __init create_domUs(void)
         if ( !dt_device_is_compatible(node, "xen,domain") )
             continue;
 
-        if ( dt_property_read_bool(node, "vpl011") )
-            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
-
         if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
             panic("Missing property 'cpus' for domain %s\n",
                   dt_node_name(node));
 
+        if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
+        {
+            d_cfg.arch.nr_spis = gic_number_lines() - 32;
+
+            /*
+             * vpl011 uses one emulated SPI. If vpl011 is requested, make
+             * sure that we allocate enough SPIs for it.
+             */
+            if ( dt_property_read_bool(node, "vpl011") )
+                d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
+                                         GUEST_VPL011_SPI - 32 + 1);
+        }
+
         d = domain_create(++max_init_domid, &d_cfg, false);
         if ( IS_ERR(d) )
             panic("Error creating domain %s\n", dt_node_name(node));