[Xen-devel,05/11] xen/arm: vpl011: Initialize nr_spis in vgic_init in Xen to atleast 1

Message ID 1487676368-22356-6-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur Feb. 21, 2017, 11:26 a.m.
Ensure that nr_spis intialized in in vgic_init is atleast 1 to allow allocation of
pl011 spi virq.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 xen/arch/arm/vgic.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Konrad Rzeszutek Wilk March 3, 2017, 8:49 p.m. | #1
On Tue, Feb 21, 2017 at 04:56:02PM +0530, Bhupinder Thakur wrote:
> Ensure that nr_spis intialized in in vgic_init is atleast 1 to allow allocation of

s/atleast/at least/

> pl011 spi virq.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/vgic.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 364d5f0..614b3ec 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -121,6 +121,11 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>      /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
>      if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>          return -EINVAL;
> +#ifdef CONFIG_VPL011_CONSOLE
> +    /* Atleast 1 spi should be available for assigning to vpl011 */

Add period at the end please.

> +    else if ( nr_spis < (1020 - NR_LOCAL_IRQS) )
> +        nr_spis += 1;

So say the nr_spis is 988, you increase it to 989 but that means
the first check which checked against that is not being enforced.

Is that OK? Should it be reflected somewhere in a documentation?
Perhaps in the xen_arch_domainconfig structure to mention this?

> +#endif
>  
>      d->arch.vgic.nr_spis = nr_spis;
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
Julien Grall March 5, 2017, 12:51 p.m. | #2
Hi Bhupinder,

Commit title: s/atleat/at least/

On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
> Ensure that nr_spis intialized in in vgic_init is atleast 1 to allow allocation of

s/intialized/initialized/ and again s/atleast/at least/

> pl011 spi virq.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/vgic.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 364d5f0..614b3ec 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -121,6 +121,11 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>      /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
>      if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>          return -EINVAL;
> +#ifdef CONFIG_VPL011_CONSOLE
> +    /* Atleast 1 spi should be available for assigning to vpl011 */
> +    else if ( nr_spis < (1020 - NR_LOCAL_IRQS) )
> +        nr_spis += 1;
> +#endif

Please don't do that. The toolstack should allocated the correct number 
of SPIs depending on the configuration of the guest.

Cheers,
Bhupinder Thakur March 16, 2017, 6:50 a.m. | #3
Hi,

On 5 March 2017 at 13:51, Julien Grall <julien.grall@arm.com> wrote:
> Hi Bhupinder,
>
> Commit title: s/atleat/at least/
>
> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
>>
>> Ensure that nr_spis intialized in in vgic_init is atleast 1 to allow
>> allocation of
>
>
> s/intialized/initialized/ and again s/atleast/at least/
>
>> pl011 spi virq.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>  xen/arch/arm/vgic.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 364d5f0..614b3ec 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -121,6 +121,11 @@ int domain_vgic_init(struct domain *d, unsigned int
>> nr_spis)
>>      /* Limit the number of virtual SPIs supported to (1020 - 32) = 988
>> */
>>      if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>>          return -EINVAL;
>> +#ifdef CONFIG_VPL011_CONSOLE
>> +    /* Atleast 1 spi should be available for assigning to vpl011 */
>> +    else if ( nr_spis < (1020 - NR_LOCAL_IRQS) )
>> +        nr_spis += 1;
>> +#endif
>
>
> Please don't do that. The toolstack should allocated the correct number of
> SPIs depending on the configuration of the guest.
>
Currently there is one parameter "irqs", which can be specified in the
guest configuration file to specify the list of physical irqs allowed
to a guest. The nr_spis is set to the maximum value of the irq
specified in the irqs list. Can I use this parameter to specify the
irq for pl011? I think not because this parameter is related to
physical irqs to be allowed to a guest.

The other option is to reserve a SPI for pl011 at compile time and use
that value. Let me know.

> Cheers,
>
> --
> Julien Grall
Julien Grall March 16, 2017, 8:24 a.m. | #4
On 03/16/2017 06:50 AM, Bhupinder Thakur wrote:
> Hi,

Hi Bhupinder,

> On 5 March 2017 at 13:51, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Bhupinder,
>>
>> Commit title: s/atleat/at least/
>>
>> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
>>>
>>> Ensure that nr_spis intialized in in vgic_init is atleast 1 to allow
>>> allocation of
>>
>>
>> s/intialized/initialized/ and again s/atleast/at least/
>>
>>> pl011 spi virq.
>>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>> ---
>>>  xen/arch/arm/vgic.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index 364d5f0..614b3ec 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -121,6 +121,11 @@ int domain_vgic_init(struct domain *d, unsigned int
>>> nr_spis)
>>>      /* Limit the number of virtual SPIs supported to (1020 - 32) = 988
>>> */
>>>      if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>>>          return -EINVAL;
>>> +#ifdef CONFIG_VPL011_CONSOLE
>>> +    /* Atleast 1 spi should be available for assigning to vpl011 */
>>> +    else if ( nr_spis < (1020 - NR_LOCAL_IRQS) )
>>> +        nr_spis += 1;
>>> +#endif
>>
>>
>> Please don't do that. The toolstack should allocated the correct number of
>> SPIs depending on the configuration of the guest.
>>
> Currently there is one parameter "irqs", which can be specified in the
> guest configuration file to specify the list of physical irqs allowed
> to a guest. The nr_spis is set to the maximum value of the irq
> specified in the irqs list. Can I use this parameter to specify the
> irq for pl011? I think not because this parameter is related to
> physical irqs to be allowed to a guest.

nr_spis in the DOMCTL create domain indicates the number of SPIs that 
will be available in the GIC emulation and exposed to the guest via 
GICD_TYPER. The virtual SPIs will be routed in a second step (see 
DOMCTL_bind_pt_irq.

>
> The other option is to reserve a SPI for pl011 at compile time and use
> that value. Let me know.

Whilst I am ok to have the pl011 SPI number hardcoded, I don't like the 
approach taken in this patch because the toolstack is in charge of the 
guest layout (interrupt, memory...) and not the hypervisor.

The values are hardcoded today because we decided to do a fix layout for 
simplicity. It is likely to be changed in the future.

The toolstack knows how much memory the user requested, the list of 
devices available... So it is the goal of the toolstack to bump the 
number of SPIs before creating the domain if a PL011 will be exposed.

Also, the interaction between the pl011 and the parameter "irqs" in the 
domain configuration file will need to be documented. By that I mean 
explaining from which number the SPIs will be allocated when choosing a 
pl011 enabling.

Note the probably want to allow the user to choose the pl011 IRQ and 
MMIO region. If he doesn't provide any, we would use the default value.

Cheers,
Bhupinder Thakur March 16, 2017, 10:31 a.m. | #5
On 16 March 2017 at 13:54, Julien Grall <julien.grall@arm.com> wrote:
>>
>> The other option is to reserve a SPI for pl011 at compile time and use
>> that value. Let me know.
>
>
> Whilst I am ok to have the pl011 SPI number hardcoded, I don't like the
> approach taken in this patch because the toolstack is in charge of the guest
> layout (interrupt, memory...) and not the hypervisor.
>
> The values are hardcoded today because we decided to do a fix layout for
> simplicity. It is likely to be changed in the future.
>
> The toolstack knows how much memory the user requested, the list of devices
> available... So it is the goal of the toolstack to bump the number of SPIs
> before creating the domain if a PL011 will be exposed.
>
> Also, the interaction between the pl011 and the parameter "irqs" in the
> domain configuration file will need to be documented. By that I mean
> explaining from which number the SPIs will be allocated when choosing a
> pl011 enabling.
>
> Note the probably want to allow the user to choose the pl011 IRQ and MMIO
> region. If he doesn't provide any, we would use the default value.
>
> Cheers,

We can follow the convention that when pl011 is enabled then the last
irq in the "irqs" list will be used as the pl011 irq. I believe that
the irqs mentioned in the "irqs" list will be reserved by the
hypervisor when the domain is created and we need not allocate a SPI
separately as we are doing currently.

One more parameter will be added "pl011" in the configuration to
indicate whether pl011 is enabled or not. By default it will disabled.

With this change, we can remove the pl011 virq HVM parameter.

Regards,
Bhupinder
Julien Grall March 16, 2017, 1:24 p.m. | #6
Hi Bhupinder,

On 03/16/2017 10:31 AM, Bhupinder Thakur wrote:
> On 16 March 2017 at 13:54, Julien Grall <julien.grall@arm.com> wrote:
>>>
>>> The other option is to reserve a SPI for pl011 at compile time and use
>>> that value. Let me know.
>>
>>
>> Whilst I am ok to have the pl011 SPI number hardcoded, I don't like the
>> approach taken in this patch because the toolstack is in charge of the guest
>> layout (interrupt, memory...) and not the hypervisor.
>>
>> The values are hardcoded today because we decided to do a fix layout for
>> simplicity. It is likely to be changed in the future.
>>
>> The toolstack knows how much memory the user requested, the list of devices
>> available... So it is the goal of the toolstack to bump the number of SPIs
>> before creating the domain if a PL011 will be exposed.
>>
>> Also, the interaction between the pl011 and the parameter "irqs" in the
>> domain configuration file will need to be documented. By that I mean
>> explaining from which number the SPIs will be allocated when choosing a
>> pl011 enabling.
>>
>> Note the probably want to allow the user to choose the pl011 IRQ and MMIO
>> region. If he doesn't provide any, we would use the default value.
>>
>> Cheers,
>
> We can follow the convention that when pl011 is enabled then the last
> irq in the "irqs" list will be used as the pl011 irq. I believe that
> the irqs mentioned in the "irqs" list will be reserved by the
> hypervisor when the domain is created and we need not allocate a SPI
> separately as we are doing currently.

I don't understand what you mean here. If you speak about the xl 
parameter "irqs", it is a list of physical IRQs and not virtual IRQs. 
Furthermore, we would like to keep to avoid a normal user to do more 
than specifying "pl011" or else on in the guest configuration file.

So could you clarify what you mean?

Cheers,
Bhupinder Thakur March 20, 2017, 4:29 p.m. | #7
On 16 March 2017 at 18:54, Julien Grall <julien.grall@arm.com> wrote:
> Hi Bhupinder,
>
> On 03/16/2017 10:31 AM, Bhupinder Thakur wrote:
>>
>> On 16 March 2017 at 13:54, Julien Grall <julien.grall@arm.com> wrote:
>>>>
>>>>
>>>> The other option is to reserve a SPI for pl011 at compile time and use
>>>> that value. Let me know.
>>>
>>>
>>>
>>> Whilst I am ok to have the pl011 SPI number hardcoded, I don't like the
>>> approach taken in this patch because the toolstack is in charge of the
>>> guest
>>> layout (interrupt, memory...) and not the hypervisor.
>>>
>>> The values are hardcoded today because we decided to do a fix layout for
>>> simplicity. It is likely to be changed in the future.
>>>
>>> The toolstack knows how much memory the user requested, the list of
>>> devices
>>> available... So it is the goal of the toolstack to bump the number of
>>> SPIs
>>> before creating the domain if a PL011 will be exposed.
>>>
>>> Also, the interaction between the pl011 and the parameter "irqs" in the
>>> domain configuration file will need to be documented. By that I mean
>>> explaining from which number the SPIs will be allocated when choosing a
>>> pl011 enabling.
>>>
>>> Note the probably want to allow the user to choose the pl011 IRQ and MMIO
>>> region. If he doesn't provide any, we would use the default value.
>>>
>>> Cheers,
>>
>>
>> We can follow the convention that when pl011 is enabled then the last
>> irq in the "irqs" list will be used as the pl011 irq. I believe that
>> the irqs mentioned in the "irqs" list will be reserved by the
>> hypervisor when the domain is created and we need not allocate a SPI
>> separately as we are doing currently.
>
>
> I don't understand what you mean here. If you speak about the xl parameter
> "irqs", it is a list of physical IRQs and not virtual IRQs. Furthermore, we
> would like to keep to avoid a normal user to do more than specifying "pl011"
> or else on in the guest configuration file.
>
> So could you clarify what you mean?
>

Now, I am taking one option in the domU configuration file which tells
whether pl011 emulation is enabled or not. Based on this option, the
tool stack will increment the nr_spis (i have removed the nr_spis
increment logic from the hypervisor code). It will create the pl011 DT
node also based on whether this option is enabled.

Patch hide | download patch | download mbox

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 364d5f0..614b3ec 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -121,6 +121,11 @@  int domain_vgic_init(struct domain *d, unsigned int nr_spis)
     /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
     if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
         return -EINVAL;
+#ifdef CONFIG_VPL011_CONSOLE
+    /* Atleast 1 spi should be available for assigning to vpl011 */
+    else if ( nr_spis < (1020 - NR_LOCAL_IRQS) )
+        nr_spis += 1;
+#endif
 
     d->arch.vgic.nr_spis = nr_spis;