diff mbox series

[v2,2/7] xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page tables

Message ID d52c84417ae4aedb8ce9f73dfa2340fceea137a4.1733937787.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series Unflattening and relocation of host device tree | expand

Commit Message

Oleksii Kurochko Dec. 11, 2024, 5:27 p.m. UTC 
Introduce the destroy_xen_mappings() function, which removes page
mappings in Xen's page tables between a start address s and an end
address e.
The function ensures that both s and e are page-aligned
and verifies that the start address is less than or equal to the end
address before calling pt_update() to invalidate the mappings.
The pt_update() function is called with INVALID_MFN and PTE_VALID=0
in the flags, which tell pt_update() to remove mapping. No additional
ASSERT() is required to check these arguments, as they are hardcoded in
the call to pt_update() within destroy_xen_mappings().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V2:
 - Drop ASSERT(s <= e).
 - Update implementation of destroy_xen_mappings() to avoid calling of
   pt_update() when start_addr >= end_addr and return -EINVAL.
 - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
---
 xen/arch/riscv/mm.c | 6 ------
 xen/arch/riscv/pt.c | 8 ++++++++
 2 files changed, 8 insertions(+), 6 deletions(-)

Comments

Jan Beulich Dec. 12, 2024, 11:43 a.m. UTC | #1 
On 11.12.2024 18:27, Oleksii Kurochko wrote:
> Introduce the destroy_xen_mappings() function, which removes page
> mappings in Xen's page tables between a start address s and an end
> address e.
> The function ensures that both s and e are page-aligned
> and verifies that the start address is less than or equal to the end
> address before calling pt_update() to invalidate the mappings.
> The pt_update() function is called with INVALID_MFN and PTE_VALID=0
> in the flags, which tell pt_update() to remove mapping. No additional
> ASSERT() is required to check these arguments, as they are hardcoded in
> the call to pt_update() within destroy_xen_mappings().
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Apparently I just shouldn't provide advance acks, when ...

> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
>      return pt_update(virt, mfn, nr_mfns, flags);
>  }
>  
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> +{
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> +
> +    return ( s < e ) ? pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0) : -EINVAL;

... then you introduce basic style violations like the excess blanks here.

Jan
Oleksii Kurochko Dec. 13, 2024, 12:54 p.m. UTC | #2 
On 12/12/24 12:43 PM, Jan Beulich wrote:
> On 11.12.2024 18:27, Oleksii Kurochko wrote:
>> Introduce the destroy_xen_mappings() function, which removes page
>> mappings in Xen's page tables between a start address s and an end
>> address e.
>> The function ensures that both s and e are page-aligned
>> and verifies that the start address is less than or equal to the end
>> address before calling pt_update() to invalidate the mappings.
>> The pt_update() function is called with INVALID_MFN and PTE_VALID=0
>> in the flags, which tell pt_update() to remove mapping. No additional
>> ASSERT() is required to check these arguments, as they are hardcoded in
>> the call to pt_update() within destroy_xen_mappings().
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> Acked-by: Jan Beulich<jbeulich@suse.com>
> Apparently I just shouldn't provide advance acks, when ...
>
>> --- a/xen/arch/riscv/pt.c
>> +++ b/xen/arch/riscv/pt.c
>> @@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
>>       return pt_update(virt, mfn, nr_mfns, flags);
>>   }
>>   
>> +int destroy_xen_mappings(unsigned long s, unsigned long e)
>> +{
>> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>> +
>> +    return ( s < e ) ? pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0) : -EINVAL;
> ... then you introduce basic style violations like the excess blanks here.

I thought that I could consider ternary operator as `if` which requires spaces around condition.

~ Oleksii
diff mbox series

Patch

diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 9359dc7f33..f2bf279bac 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -360,12 +360,6 @@  int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
     return 0;
 }
 
-int destroy_xen_mappings(unsigned long s, unsigned long e)
-{
-    BUG_ON("unimplemented");
-    return -1;
-}
-
 void share_xen_page_with_guest(struct page_info *page, struct domain *d,
                                enum XENSHARE_flags flags)
 {
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index d62aceb36c..86bd9ea613 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -421,6 +421,14 @@  int map_pages_to_xen(unsigned long virt,
     return pt_update(virt, mfn, nr_mfns, flags);
 }
 
+int destroy_xen_mappings(unsigned long s, unsigned long e)
+{
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
+
+    return ( s < e ) ? pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0) : -EINVAL;
+}
+
 int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 {
     return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);