|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 2/3] arm/mpu: Introduce `v8r_el1_msa` device tree property for domains
Hi Michal,
thanks for your review
>>
>> +static int __init
>> +domu_dt_v8r_el1_msa_parse(const struct dt_device_node *node,
>> + struct xen_domctl_createdomain *d_cfg,
>> + unsigned int flags)
>> +{
>> + bool property_present = dt_property_read_bool(node, "v8r_el1_msa");
> I know it's present in the code for SCI, but it's not really necessary to do
> the
> DT parsing twice (once for boot, second for string given that
> dt_property_read_string returns -EINVAL if no property found).
>
> You could move this bool check ...
>
>> +
>> + if ( !IS_ENABLED(CONFIG_MPU) )
>> + {
>> + d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
>> +
>> + if ( !property_present )
> ... here.
>
>> + return 0;
>> +
>> + printk(XENLOG_ERR
>> + "Not supported 'v8r_el1_msa' DT property found for domain
>> %s\n",
>> + dt_node_full_name(node));
>> + return -EINVAL;
>> + }
>> +
>> + if ( !property_present )
>> + d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
>> + else
>> + {
>> + const char *v8r_el1_msa;
>> + int ret = dt_property_read_string(node, "v8r_el1_msa",
>> &v8r_el1_msa);
>> +
>> + if ( ret )
>> + return ret;
>> +
>> + if ( !strcmp(v8r_el1_msa, "mpu") )
>> + d_cfg->arch.v8r_el1_msa =
>> XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
>> + else if ( !strcmp(v8r_el1_msa, "mmu") )
>> + d_cfg->arch.v8r_el1_msa =
>> XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA;
>> + else
>> + {
>> + printk(XENLOG_ERR
>> + "v8r_el1_msa value (%s) not valid for domain %s\n",
>> + v8r_el1_msa, dt_node_full_name(node));
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + switch ( d_cfg->arch.v8r_el1_msa )
>> + {
>> + case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA:
>> + if ( !(flags & CDF_staticmem) || !(flags & CDF_directmap) )
>> + {
>> + printk(XENLOG_ERR
>> + "PMSA is not valid for domain (%s) without static
>> allocation and direct map (v8r_el1_msa)\n",
>> + dt_node_full_name(node));
>> + return -EINVAL;
>> + }
>> + break;
>> +
>> + case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA:
>> + if ( !has_v8r_vmsa_support() )
>> + {
>> + printk(XENLOG_ERR
>> + "Platform doesn't support VMSA at EL1 (v8r_el1_msa)\n");
>> + return -EINVAL;
>> + }
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
> This does not look very clean. How about:
>
> static int __init
> domu_dt_v8r_el1_msa_parse(const struct dt_device_node *node,
> struct xen_domctl_createdomain *d_cfg,
> unsigned int flags)
> {
> const char *value;
> int ret;
>
> if ( !IS_ENABLED(CONFIG_MPU) )
> {
> d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
>
> if ( !dt_property_read_bool(node, "v8r_el1_msa") )
> return 0;
>
> printk(XENLOG_ERR
> "v8r_el1_msa not supported on this build for domain %s\n",
> dt_node_full_name(node));
> return -EINVAL;
> }
>
> ret = dt_property_read_string(node, "v8r_el1_msa", &value);
> /* property absent: PMSA is the default */
> if ( ret == -EINVAL )
> value = "mpu";
> else if ( ret )
> return ret;
>
> if ( !strcmp(value, "mpu") )
> {
> if ( !(flags & CDF_staticmem) || !(flags & CDF_directmap) )
> {
> printk(XENLOG_ERR
> "v8r_el1_msa=mpu requires static-mem and direct-map for
> domain %s\n",
> dt_node_full_name(node));
> return -EINVAL;
> }
> d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
> return 0;
> }
>
> if ( !strcmp(value, "mmu") )
> {
> if ( !has_v8r_vmsa_support() )
> {
> printk(XENLOG_ERR
> "v8r_el1_msa=mmu unsupported by platform for domain %s\n",
> dt_node_full_name(node));
> return -EINVAL;
> }
> d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA;
> return 0;
> }
>
> printk(XENLOG_ERR "v8r_el1_msa value '%s' not valid for domain %s\n",
> value, dt_node_full_name(node));
> return -EINVAL;
> }
ack
>
>
>> +}
>> +
>> int __init arch_parse_dom0less_node(struct dt_device_node *node,
>> struct boot_domain *bd)
>> {
>> @@ -315,6 +386,9 @@ int __init arch_parse_dom0less_node(struct
>> dt_device_node *node,
>> if ( domu_dt_sci_parse(node, d_cfg) )
>> panic("Error getting SCI configuration\n");
>>
>> + if ( domu_dt_v8r_el1_msa_parse(node, d_cfg, flags) )
>> + panic("Error getting v8r_el1_msa configuration\n");
>> +
>> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg->arch.nr_spis) )
>> {
>> int vpl011_virq = GUEST_VPL011_SPI;
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 26380a807cad..e579c3b1bb3c 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -18,6 +18,7 @@
>> #include <asm/cpuerrata.h>
>> #include <asm/cpufeature.h>
>> #include <asm/current.h>
>> +#include <asm/domain_build.h>
> You don't seem to use anything from this header.
Yep my bad, leftover
>
>> #include <asm/event.h>
>> #include <asm/gic.h>
>> #include <asm/guest_atomics.h>
>> @@ -538,6 +539,24 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
>> v->arch.hcr_el2 |= HCR_RW;
>> }
>>
>> +static bool v8r_el1_msa_domain_sanitise_config(
>> + const struct xen_domctl_createdomain *config)
>> +{
>> + uint8_t v8r_el1_msa = config->arch.v8r_el1_msa;
> That is not a useful assignment.
yeah it was only to shorten the line
>
>> +
>> + if ( !IS_ENABLED(CONFIG_MPU) )
>> + return v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
>> +
>> + if ( IS_ENABLED(CONFIG_ARM_32) )
>> + return v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
>> +
>> + if ( IS_ENABLED(CONFIG_ARM_64) )
>> + return (v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA) ||
>> + (v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA);
>> +
>> + return false;
> Arm32 and Arm64 are mutually exclusive, so this line is unreachable.
> How about:
> static bool v8r_el1_msa_domain_sanitise_config(
> const struct xen_domctl_createdomain *config)
> {
> switch ( config->arch.v8r_el1_msa )
> {
> case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE:
> return !IS_ENABLED(CONFIG_MPU);
>
> case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA:
> return IS_ENABLED(CONFIG_MPU);
>
> case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA:
> return IS_ENABLED(CONFIG_MPU) && IS_ENABLED(CONFIG_ARM_64);
>
> default:
> return false;
> }
> }
ack
>>
>> diff --git a/xen/arch/arm/include/asm/domain_build.h
>> b/xen/arch/arm/include/asm/domain_build.h
>> index 6674dac5e2f8..13e88fc0891b 100644
>> --- a/xen/arch/arm/include/asm/domain_build.h
>> +++ b/xen/arch/arm/include/asm/domain_build.h
>> @@ -19,6 +19,16 @@ int prepare_acpi(struct domain *d, struct kernel_info
>> *kinfo);
>>
>> int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
>>
>> +#ifdef CONFIG_MPU
> You could do: && CONFIG_ARM64 and then ...
>
>> +/* Utility function to determine if an Armv8-R processor supports VMSA. */
>> +bool has_v8r_vmsa_support(void);
>> +#else
>> +static inline bool has_v8r_vmsa_support(void)
>> +{
>> + return false;
>> +}
>> +#endif /* CONFIG_MPU */
>> +
>> #endif
>>
>> /*
>> diff --git a/xen/arch/arm/mpu/arm32/mm.c b/xen/arch/arm/mpu/arm32/mm.c
>> index a4673c351141..702bea804acd 100644
>> --- a/xen/arch/arm/mpu/arm32/mm.c
>> +++ b/xen/arch/arm/mpu/arm32/mm.c
>> @@ -5,6 +5,8 @@
>> #include <asm/mpu.h>
>> #include <asm/sysregs.h>
>> #include <asm/system.h>
>> +#include <public/arch-arm.h>
>> +#include <public/domctl.h>
> You don't seem to add anything from these headers.
Yep, leftover
>
>>
>> #define GENERATE_WRITE_PR_REG_CASE(num, pr) \
>> case num: \
>> @@ -38,6 +40,11 @@
>> break; \
>> }
>>
>> +bool has_v8r_vmsa_support(void)
>> +{
>> + return false;
>> +}
> ... and then you could get rid of this stub
ack
>> +
>> /*
>> * Armv8-R supports direct access and indirect access to the MPU regions
>> through
>> * registers:
>> diff --git a/xen/arch/arm/mpu/arm64/mm.c b/xen/arch/arm/mpu/arm64/mm.c
>> index ed643cad4073..b8abcc6f7bc6 100644
>> --- a/xen/arch/arm/mpu/arm64/mm.c
>> +++ b/xen/arch/arm/mpu/arm64/mm.c
>> @@ -5,6 +5,8 @@
>> #include <asm/mpu.h>
>> #include <asm/sysregs.h>
>> #include <asm/system.h>
>> +#include <public/arch-arm.h>
>> +#include <public/domctl.h>
> You don't seem to add anything from these headers.
Yep, leftover
Will respin with the fixes soon.
Cheers,
Luca
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |