| Message ID | 20210302150002.3685113-28-qperret@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: A stage 2 for the host | expand |
On Tue, Mar 02, 2021 at 02:59:57PM +0000, Quentin Perret wrote: > In order to ease its re-use in other code paths, refactor > stage2_map_set_prot_attr() to not depend on a stage2_map_data struct. > No functional change intended. > > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/kvm/hyp/pgtable.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index 8e7059fcfd40..8aa01a9e2603 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -494,8 +494,7 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) > return vtcr; > } > > -static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, > - struct stage2_map_data *data) > +static kvm_pte_t stage2_get_prot_attr(enum kvm_pgtable_prot prot) > { > bool device = prot & KVM_PGTABLE_PROT_DEVICE; > kvm_pte_t attr = device ? PAGE_S2_MEMATTR(DEVICE_nGnRE) : > @@ -504,15 +503,15 @@ static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, > > if (prot & KVM_PGTABLE_PROT_NONE) { > if (prot != KVM_PGTABLE_PROT_NONE) > - return -EINVAL; > + return 0; Hmm, does the architecture actually say that having all these attributes as 0 is illegal? If not, I think it would be better to keep the int return code and replace the 'data' parameter with a pointer to a kvm_pte_t. Does that work? Will
On Thursday 04 Mar 2021 at 20:03:36 (+0000), Will Deacon wrote: > On Tue, Mar 02, 2021 at 02:59:57PM +0000, Quentin Perret wrote: > > In order to ease its re-use in other code paths, refactor > > stage2_map_set_prot_attr() to not depend on a stage2_map_data struct. > > No functional change intended. > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 19 ++++++++----------- > > 1 file changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index 8e7059fcfd40..8aa01a9e2603 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -494,8 +494,7 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) > > return vtcr; > > } > > > > -static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, > > - struct stage2_map_data *data) > > +static kvm_pte_t stage2_get_prot_attr(enum kvm_pgtable_prot prot) > > { > > bool device = prot & KVM_PGTABLE_PROT_DEVICE; > > kvm_pte_t attr = device ? PAGE_S2_MEMATTR(DEVICE_nGnRE) : > > @@ -504,15 +503,15 @@ static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, > > > > if (prot & KVM_PGTABLE_PROT_NONE) { > > if (prot != KVM_PGTABLE_PROT_NONE) > > - return -EINVAL; > > + return 0; > > Hmm, does the architecture actually say that having all these attributes > as 0 is illegal? Hmm, that's a good point, that might not be the case. I assumed we would have no use for this, but there we can easily avoid the restriction so... > If not, I think it would be better to keep the int return > code and replace the 'data' parameter with a pointer to a kvm_pte_t. > > Does that work? I think so yes, I'll fix it up. Cheers, Quentin
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index 8e7059fcfd40..8aa01a9e2603 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -494,8 +494,7 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) return vtcr; } -static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, - struct stage2_map_data *data) +static kvm_pte_t stage2_get_prot_attr(enum kvm_pgtable_prot prot) { bool device = prot & KVM_PGTABLE_PROT_DEVICE; kvm_pte_t attr = device ? PAGE_S2_MEMATTR(DEVICE_nGnRE) : @@ -504,15 +503,15 @@ static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, if (prot & KVM_PGTABLE_PROT_NONE) { if (prot != KVM_PGTABLE_PROT_NONE) - return -EINVAL; + return 0; attr |= KVM_PTE_LEAF_SW_BIT_PROT_NONE; - goto out; + return attr; } if (!(prot & KVM_PGTABLE_PROT_X)) attr |= KVM_PTE_LEAF_ATTR_HI_S2_XN; else if (device) - return -EINVAL; + return 0; if (prot & KVM_PGTABLE_PROT_R) attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R; @@ -523,9 +522,7 @@ static int stage2_map_set_prot_attr(enum kvm_pgtable_prot prot, attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_LO_S2_SH, sh); attr |= KVM_PTE_LEAF_ATTR_LO_S2_AF; -out: - data->attr = attr; - return 0; + return attr; } static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, @@ -708,9 +705,9 @@ int kvm_pgtable_stage2_map(struct kvm_pgtable *pgt, u64 addr, u64 size, .arg = &map_data, }; - ret = stage2_map_set_prot_attr(prot, &map_data); - if (ret) - return ret; + map_data.attr = stage2_get_prot_attr(prot); + if (!map_data.attr) + return -EINVAL; ret = kvm_pgtable_walk(pgt, addr, size, &walker); dsb(ishst);
In order to ease its re-use in other code paths, refactor stage2_map_set_prot_attr() to not depend on a stage2_map_data struct. No functional change intended. Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/pgtable.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)