[Xen-devel] [PATCH v4 0/3] nvmx: implement support for MSR bitmaps

[Xen-devel] [PATCH v4 0/3] nvmx: implement support for MSR bitmaps

[Roger Pau Monne]

Hello,

Current nested VMX code advertises support for the MSR bitmap feature,
yet the implementation isn't done. Previous to this series Xen just maps
the nested guest MSR bitmap (as set by L1) and that's it, the L2 guest
ends up using the L1 MSR bitmap.

This series adds handling of the L2 MSR bitmap and merging with the L1
MSR bitmap and loading it into the nested guest VMCS.

Patch #3 makes sure the x2APIC MSR range is always trapped, or else a
guest with nested virtualization enabled could manage to access some of
the x2APIC MSR registers from the host.

Thanks, Roger.

Roger Pau Monne (3):
  nvmx: implement support for MSR bitmaps
  bitmap: import bitmap_{set/clear} from Linux 5.5
  nvmx: always trap accesses to x2APIC MSRs

 xen/arch/x86/hvm/vmx/vvmx.c        | 80 ++++++++++++++++++++++++++++--
 xen/common/bitmap.c                | 41 +++++++++++++++
 xen/include/asm-x86/hvm/vmx/vvmx.h |  3 +-
 xen/include/xen/bitmap.h           | 39 +++++++++++++++
 4 files changed, 158 insertions(+), 5 deletions(-)

-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 1/3] nvmx: implement support for MSR bitmaps

[Roger Pau Monne]

Current implementation of nested VMX has a half baked handling of MSR
bitmaps for the L1 VMM: it maps the L1 VMM provided MSR bitmap, but
doesn't actually load it into the nested vmcs, and thus the nested
guest vmcs ends up using the same MSR bitmap as the L1 VMM.

This is wrong as there's no assurance that the set of features enabled
for the L1 vmcs are the same that L1 itself is going to use in the
nested vmcs, and thus can lead to misconfigurations.

For example L1 vmcs can use x2APIC virtualization and virtual
interrupt delivery, and thus some x2APIC MSRs won't be trapped so that
they can be handled directly by the hardware using virtualization
extensions. On the other hand, the nested vmcs created by L1 VMM might
not use any of such features, so using a MSR bitmap that doesn't trap
accesses to the x2APIC MSRs will be leaking them to the underlying
hardware.

Fix this by crafting a merged MSR bitmap between the one used by L1
and the nested guest.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
This seems better than what's done currently, but TBH there's a lot of
work to be done in nvmx in order to make it functional and secure that
I'm not sure whether building on top of the current implementation is
something sane to do, or it would be better to start from scratch and
re-implement nvmx to just support the minimum required set of VTx
features in a sane and safe way.
---
Changes since v3:
 - Free the merged MSR bitmap page in nvmx_domain_relinquish_resources.

Changes since v2:
 - Pass shadow_ctrl into update_msrbitmap, and check there if
   CPU_BASED_ACTIVATE_MSR_BITMAP is set.
 - Do not enable MSR bitmap unless it's enabled in both L1 and L2.
 - Rename L1 guest to L2 in nestedvmx struct comment.

Changes since v1:
 - Split the x2APIC MSR fix into a separate patch.
 - Move setting MSR_BITMAP vmcs field into load_vvmcs_host_state for
   virtual vmexit.
 - Allocate memory with MEMF_no_owner.
 - Use tabs to align comment of the nestedvmx struct field.
---
 xen/arch/x86/hvm/vmx/vvmx.c        | 73 ++++++++++++++++++++++++++++--
 xen/include/asm-x86/hvm/vmx/vvmx.h |  3 +-
 2 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 47eee1e5b9..46c51a95b9 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -128,6 +128,16 @@ int nvmx_vcpu_initialise(struct vcpu *v)
         unmap_domain_page(vw);
     }
 
+    if ( cpu_has_vmx_msr_bitmap )
+    {
+        nvmx->msr_merged = alloc_domheap_page(d, MEMF_no_owner);
+        if ( !nvmx->msr_merged )
+        {
+            gdprintk(XENLOG_ERR, "nest: allocation for MSR bitmap failed\n");
+            return -ENOMEM;
+        }
+    }
+
     nvmx->ept.enabled = 0;
     nvmx->guest_vpid = 0;
     nvmx->vmxon_region_pa = INVALID_PADDR;
@@ -183,13 +193,27 @@ void nvmx_vcpu_destroy(struct vcpu *v)
         v->arch.hvm.vmx.vmwrite_bitmap = NULL;
     }
 }
- 
+
+void vcpu_relinquish_resources(struct vcpu *v)
+{
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+
+    if ( nvmx->msr_merged )
+    {
+        free_domheap_page(nvmx->msr_merged);
+        nvmx->msr_merged = NULL;
+    }
+}
+
 void nvmx_domain_relinquish_resources(struct domain *d)
 {
     struct vcpu *v;
 
     for_each_vcpu ( d, v )
+    {
         nvmx_purge_vvmcs(v);
+        vcpu_relinquish_resources(v);
+    }
 }
 
 int nvmx_vcpu_reset(struct vcpu *v)
@@ -548,6 +572,35 @@ unsigned long *_shadow_io_bitmap(struct vcpu *v)
     return nestedhvm_vcpu_iomap_get(port80, portED);
 }
 
+static void update_msrbitmap(struct vcpu *v, uint32_t shadow_ctrl)
+{
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+    struct vmx_msr_bitmap *msr_bitmap;
+
+    if ( !(shadow_ctrl & CPU_BASED_ACTIVATE_MSR_BITMAP) ||
+         !nvmx->msrbitmap )
+       return;
+
+    msr_bitmap = __map_domain_page(nvmx->msr_merged);
+
+    bitmap_or(msr_bitmap->read_low, nvmx->msrbitmap->read_low,
+              v->arch.hvm.vmx.msr_bitmap->read_low,
+              sizeof(msr_bitmap->read_low) * 8);
+    bitmap_or(msr_bitmap->read_high, nvmx->msrbitmap->read_high,
+              v->arch.hvm.vmx.msr_bitmap->read_high,
+              sizeof(msr_bitmap->read_high) * 8);
+    bitmap_or(msr_bitmap->write_low, nvmx->msrbitmap->write_low,
+              v->arch.hvm.vmx.msr_bitmap->write_low,
+              sizeof(msr_bitmap->write_low) * 8);
+    bitmap_or(msr_bitmap->write_high, nvmx->msrbitmap->write_high,
+              v->arch.hvm.vmx.msr_bitmap->write_high,
+              sizeof(msr_bitmap->write_high) * 8);
+
+    unmap_domain_page(msr_bitmap);
+
+    __vmwrite(MSR_BITMAP, page_to_maddr(nvmx->msr_merged));
+}
+
 void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl)
 {
     u32 pio_cntrl = (CPU_BASED_ACTIVATE_IO_BITMAP
@@ -558,10 +611,17 @@ void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl)
     shadow_cntrl = __n2_exec_control(v);
     pio_cntrl &= shadow_cntrl;
     /* Enforce the removed features */
-    shadow_cntrl &= ~(CPU_BASED_ACTIVATE_MSR_BITMAP
-                      | CPU_BASED_ACTIVATE_IO_BITMAP
+    shadow_cntrl &= ~(CPU_BASED_ACTIVATE_IO_BITMAP
                       | CPU_BASED_UNCOND_IO_EXITING);
-    shadow_cntrl |= host_cntrl;
+    /*
+     * Do NOT enforce the MSR bitmap currently used by L1, as certain hardware
+     * virtualization features require specific MSR bitmap settings, but
+     * without the guest also using these same features the bitmap could be
+     * leaking through unwanted MSR accesses.
+     */
+    shadow_cntrl |= host_cntrl & ~CPU_BASED_ACTIVATE_MSR_BITMAP;
+    if ( !(shadow_cntrl & host_cntrl & CPU_BASED_ACTIVATE_MSR_BITMAP) )
+      shadow_cntrl &= ~CPU_BASED_ACTIVATE_MSR_BITMAP;
     if ( pio_cntrl == CPU_BASED_UNCOND_IO_EXITING ) {
         /* L1 VMM intercepts all I/O instructions */
         shadow_cntrl |= CPU_BASED_UNCOND_IO_EXITING;
@@ -584,6 +644,8 @@ void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl)
         __vmwrite(IO_BITMAP_B, virt_to_maddr(bitmap) + PAGE_SIZE);
     }
 
+    update_msrbitmap(v, shadow_cntrl);
+
     /* TODO: change L0 intr window to MTF or NMI window */
     __vmwrite(CPU_BASED_VM_EXEC_CONTROL, shadow_cntrl);
 }
@@ -1278,6 +1340,9 @@ static void load_vvmcs_host_state(struct vcpu *v)
     hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset, 0);
 
     set_vvmcs(v, VM_ENTRY_INTR_INFO, 0);
+
+    if ( v->arch.hvm.vmx.exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP )
+        __vmwrite(MSR_BITMAP, virt_to_maddr(v->arch.hvm.vmx.msr_bitmap));
 }
 
 static void sync_exception_state(struct vcpu *v)
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index 6b9c4ae0b2..c41f089939 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -37,7 +37,8 @@ struct nestedvmx {
      */
     paddr_t    vmxon_region_pa;
     void       *iobitmap[2];		/* map (va) of L1 guest I/O bitmap */
-    void       *msrbitmap;		/* map (va) of L1 guest MSR bitmap */
+    struct vmx_msr_bitmap *msrbitmap;	/* map (va) of L1 guest MSR bitmap */
+    struct page_info *msr_merged;	/* merged L1 and L2 MSR bitmap */
     /* deferred nested interrupt */
     struct {
         unsigned long intr_info;
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 1/3] nvmx: implement support for MSR bitmaps

[Jan Beulich]

On 04.02.2020 18:34, Roger Pau Monne wrote:
> @@ -183,13 +193,27 @@ void nvmx_vcpu_destroy(struct vcpu *v)
>          v->arch.hvm.vmx.vmwrite_bitmap = NULL;
>      }
>  }
> - 
> +
> +void vcpu_relinquish_resources(struct vcpu *v)

static (which would be easy to add while committing, if no other
need arises for a v5 here)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Roger Pau Monne]

Import the functions and it's dependencies. Based on Linux 5.5, commit
id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/bitmap.c          | 41 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/bitops.h |  2 ++
 xen/include/xen/bitmap.h     | 38 +++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index fd070bee97..f8b243e77e 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
 #endif
 EXPORT_SYMBOL(__bitmap_weight);
 
+void __bitmap_set(unsigned long *map, unsigned int start, int len)
+{
+	unsigned long *p = map + BIT_WORD(start);
+	const unsigned int size = start + len;
+	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
+	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
+
+	while (len - bits_to_set >= 0) {
+		*p |= mask_to_set;
+		len -= bits_to_set;
+		bits_to_set = BITS_PER_LONG;
+		mask_to_set = ~0UL;
+		p++;
+	}
+	if (len) {
+		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
+		*p |= mask_to_set;
+	}
+}
+EXPORT_SYMBOL(__bitmap_set);
+
+void __bitmap_clear(unsigned long *map, unsigned int start, int len)
+{
+	unsigned long *p = map + BIT_WORD(start);
+	const unsigned int size = start + len;
+	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
+	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
+
+	while (len - bits_to_clear >= 0) {
+		*p &= ~mask_to_clear;
+		len -= bits_to_clear;
+		bits_to_clear = BITS_PER_LONG;
+		mask_to_clear = ~0UL;
+		p++;
+	}
+	if (len) {
+		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
+		*p &= ~mask_to_clear;
+	}
+}
+EXPORT_SYMBOL(__bitmap_clear);
 
 /**
  *	bitmap_find_free_region - find a contiguous aligned mem region
diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h
index 5a71afbc89..04b1530388 100644
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
 #define hweight16(x) generic_hweight16(x)
 #define hweight8(x) generic_hweight8(x)
 
+#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
+
 #endif /* _X86_BITOPS_H */
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index 4e1e690af1..f07d1f6935 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -85,6 +85,8 @@ extern int __bitmap_intersects(const unsigned long *bitmap1,
 extern int __bitmap_subset(const unsigned long *bitmap1,
 			const unsigned long *bitmap2, int bits);
 extern int __bitmap_weight(const unsigned long *bitmap, int bits);
+extern void __bitmap_set(unsigned long *map, unsigned int start, int len);
+extern void __bitmap_clear(unsigned long *map, unsigned int start, int len);
 
 extern int bitmap_find_free_region(unsigned long *bitmap, int bits, int order);
 extern void bitmap_release_region(unsigned long *bitmap, int pos, int order);
@@ -227,6 +229,42 @@ static inline int bitmap_weight(const unsigned long *src, int nbits)
 	return __bitmap_weight(src, nbits);
 }
 
+#ifdef __LITTLE_ENDIAN
+#define BITMAP_MEM_ALIGNMENT 8
+#else
+#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
+#endif
+#define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
+#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
+
+static inline void bitmap_set(unsigned long *map, unsigned int start,
+		unsigned int nbits)
+{
+	if (__builtin_constant_p(nbits) && nbits == 1)
+		__set_bit(start, map);
+	else if (__builtin_constant_p(start & BITMAP_MEM_MASK) &&
+		 IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) &&
+		 __builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
+		 IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
+		memset((char *)map + start / 8, 0xff, nbits / 8);
+	else
+		__bitmap_set(map, start, nbits);
+}
+
+static inline void bitmap_clear(unsigned long *map, unsigned int start,
+		unsigned int nbits)
+{
+	if (__builtin_constant_p(nbits) && nbits == 1)
+		__clear_bit(start, map);
+	else if (__builtin_constant_p(start & BITMAP_MEM_MASK) &&
+		 IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) &&
+		 __builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
+		 IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
+		memset((char *)map + start / 8, 0, nbits / 8);
+	else
+		__bitmap_clear(map, start, nbits);
+}
+
 #undef bitmap_switch
 #undef bitmap_bytes
 
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 04.02.2020 18:34, Roger Pau Monne wrote:
> Import the functions and it's dependencies. Based on Linux 5.5, commit
> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks for going this route; two remarks / requests:

> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>  #endif
>  EXPORT_SYMBOL(__bitmap_weight);
>  
> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
> +{
> +	unsigned long *p = map + BIT_WORD(start);
> +	const unsigned int size = start + len;
> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
> +
> +	while (len - bits_to_set >= 0) {
> +		*p |= mask_to_set;
> +		len -= bits_to_set;
> +		bits_to_set = BITS_PER_LONG;
> +		mask_to_set = ~0UL;
> +		p++;
> +	}
> +	if (len) {
> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
> +		*p |= mask_to_set;
> +	}
> +}
> +EXPORT_SYMBOL(__bitmap_set);
> +
> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
> +{
> +	unsigned long *p = map + BIT_WORD(start);
> +	const unsigned int size = start + len;
> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> +
> +	while (len - bits_to_clear >= 0) {
> +		*p &= ~mask_to_clear;
> +		len -= bits_to_clear;
> +		bits_to_clear = BITS_PER_LONG;
> +		mask_to_clear = ~0UL;
> +		p++;
> +	}
> +	if (len) {
> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
> +		*p &= ~mask_to_clear;
> +	}
> +}
> +EXPORT_SYMBOL(__bitmap_clear);

Despite all the other EXPORT_SYMBOL() in this file, personally I
would suggest to refrain from adding more. But I'm not going to
insist (until such time that they all get cleaned up).

> --- a/xen/include/asm-x86/bitops.h
> +++ b/xen/include/asm-x86/bitops.h
> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>  #define hweight16(x) generic_hweight16(x)
>  #define hweight8(x) generic_hweight8(x)
>  
> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)

At first I thought - why for x86 only? Then I noticed Arm has an
almost identical #define already. Which in turn made me look at
Linux, where that #define lives in a common header. I think you
want to move the Arm one. Or wait, no - Arm's isn't even
compatible with the implementations of the functions you add.
This definitely needs taking care of, perhaps by way of ignoring
my request to go this route (as getting too involved).

> @@ -227,6 +229,42 @@ static inline int bitmap_weight(const unsigned long *src, int nbits)
>  	return __bitmap_weight(src, nbits);
>  }
>  
> +#ifdef __LITTLE_ENDIAN
> +#define BITMAP_MEM_ALIGNMENT 8
> +#else
> +#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
> +#endif

For __LITTLE_ENDIAN to be consistently defined (or not), don't
you need to include <asm/byteorder.h> here?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Roger Pau Monné]

On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
> On 04.02.2020 18:34, Roger Pau Monne wrote:
> > Import the functions and it's dependencies. Based on Linux 5.5, commit
> > id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks for going this route; two remarks / requests:
> 
> > --- a/xen/common/bitmap.c
> > +++ b/xen/common/bitmap.c
> > @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
> >  #endif
> >  EXPORT_SYMBOL(__bitmap_weight);
> >  
> > +void __bitmap_set(unsigned long *map, unsigned int start, int len)
> > +{
> > +	unsigned long *p = map + BIT_WORD(start);
> > +	const unsigned int size = start + len;
> > +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
> > +
> > +	while (len - bits_to_set >= 0) {
> > +		*p |= mask_to_set;
> > +		len -= bits_to_set;
> > +		bits_to_set = BITS_PER_LONG;
> > +		mask_to_set = ~0UL;
> > +		p++;
> > +	}
> > +	if (len) {
> > +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
> > +		*p |= mask_to_set;
> > +	}
> > +}
> > +EXPORT_SYMBOL(__bitmap_set);
> > +
> > +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
> > +{
> > +	unsigned long *p = map + BIT_WORD(start);
> > +	const unsigned int size = start + len;
> > +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> > +
> > +	while (len - bits_to_clear >= 0) {
> > +		*p &= ~mask_to_clear;
> > +		len -= bits_to_clear;
> > +		bits_to_clear = BITS_PER_LONG;
> > +		mask_to_clear = ~0UL;
> > +		p++;
> > +	}
> > +	if (len) {
> > +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
> > +		*p &= ~mask_to_clear;
> > +	}
> > +}
> > +EXPORT_SYMBOL(__bitmap_clear);
> 
> Despite all the other EXPORT_SYMBOL() in this file, personally I
> would suggest to refrain from adding more. But I'm not going to
> insist (until such time that they all get cleaned up).
> 
> > --- a/xen/include/asm-x86/bitops.h
> > +++ b/xen/include/asm-x86/bitops.h
> > @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
> >  #define hweight16(x) generic_hweight16(x)
> >  #define hweight8(x) generic_hweight8(x)
> >  
> > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> 
> At first I thought - why for x86 only? Then I noticed Arm has an
> almost identical #define already. Which in turn made me look at
> Linux, where that #define lives in a common header. I think you
> want to move the Arm one. Or wait, no - Arm's isn't even
> compatible with the implementations of the functions you add.
> This definitely needs taking care of, perhaps by way of ignoring
> my request to go this route (as getting too involved).

Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
used when the bitmap is mapped to an array of 32bit type elements.

I could introduce BIT_LONG that would have the same definition on Arm
and x86, and then modify the imported functions to use it, but IMO the
right solution would be to change the Arm BIT_WORD macro to also use
BITS_PER_LONG (and adjust the callers).

This seems quite far off, so if you don't mind I would rather have the
original v3 2/2 using set_bit:

https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 05.02.2020 14:21, Roger Pau Monné wrote:
> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
>> On 04.02.2020 18:34, Roger Pau Monne wrote:
>>> Import the functions and it's dependencies. Based on Linux 5.5, commit
>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Thanks for going this route; two remarks / requests:
>>
>>> --- a/xen/common/bitmap.c
>>> +++ b/xen/common/bitmap.c
>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>>>  #endif
>>>  EXPORT_SYMBOL(__bitmap_weight);
>>>  
>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
>>> +{
>>> +	unsigned long *p = map + BIT_WORD(start);
>>> +	const unsigned int size = start + len;
>>> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
>>> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>>> +
>>> +	while (len - bits_to_set >= 0) {
>>> +		*p |= mask_to_set;
>>> +		len -= bits_to_set;
>>> +		bits_to_set = BITS_PER_LONG;
>>> +		mask_to_set = ~0UL;
>>> +		p++;
>>> +	}
>>> +	if (len) {
>>> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
>>> +		*p |= mask_to_set;
>>> +	}
>>> +}
>>> +EXPORT_SYMBOL(__bitmap_set);
>>> +
>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
>>> +{
>>> +	unsigned long *p = map + BIT_WORD(start);
>>> +	const unsigned int size = start + len;
>>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>>> +
>>> +	while (len - bits_to_clear >= 0) {
>>> +		*p &= ~mask_to_clear;
>>> +		len -= bits_to_clear;
>>> +		bits_to_clear = BITS_PER_LONG;
>>> +		mask_to_clear = ~0UL;
>>> +		p++;
>>> +	}
>>> +	if (len) {
>>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>>> +		*p &= ~mask_to_clear;
>>> +	}
>>> +}
>>> +EXPORT_SYMBOL(__bitmap_clear);
>>
>> Despite all the other EXPORT_SYMBOL() in this file, personally I
>> would suggest to refrain from adding more. But I'm not going to
>> insist (until such time that they all get cleaned up).
>>
>>> --- a/xen/include/asm-x86/bitops.h
>>> +++ b/xen/include/asm-x86/bitops.h
>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>>>  #define hweight16(x) generic_hweight16(x)
>>>  #define hweight8(x) generic_hweight8(x)
>>>  
>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>>
>> At first I thought - why for x86 only? Then I noticed Arm has an
>> almost identical #define already. Which in turn made me look at
>> Linux, where that #define lives in a common header. I think you
>> want to move the Arm one. Or wait, no - Arm's isn't even
>> compatible with the implementations of the functions you add.
>> This definitely needs taking care of, perhaps by way of ignoring
>> my request to go this route (as getting too involved).
> 
> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
> used when the bitmap is mapped to an array of 32bit type elements.
> 
> I could introduce BIT_LONG that would have the same definition on Arm
> and x86, and then modify the imported functions to use it, but IMO the
> right solution would be to change the Arm BIT_WORD macro to also use
> BITS_PER_LONG (and adjust the callers).

So do I. Julien, Stefano?

> This seems quite far off, so if you don't mind I would rather have the
> original v3 2/2 using set_bit:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html

As per my previous reply - yes, I'm okay with that, and yes,
expecting this I've also kept your patches this way in my
to-be-committed folder (pending Kevin's ack for patch 1).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Julien Grall]

On 05/02/2020 13:27, Jan Beulich wrote:
> On 05.02.2020 14:21, Roger Pau Monné wrote:
>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
>>> On 04.02.2020 18:34, Roger Pau Monne wrote:
>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit
>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> Thanks for going this route; two remarks / requests:
>>>
>>>> --- a/xen/common/bitmap.c
>>>> +++ b/xen/common/bitmap.c
>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>>>>   #endif
>>>>   EXPORT_SYMBOL(__bitmap_weight);
>>>>   
>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
>>>> +{
>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>> +	const unsigned int size = start + len;
>>>> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>>>> +
>>>> +	while (len - bits_to_set >= 0) {
>>>> +		*p |= mask_to_set;
>>>> +		len -= bits_to_set;
>>>> +		bits_to_set = BITS_PER_LONG;
>>>> +		mask_to_set = ~0UL;
>>>> +		p++;
>>>> +	}
>>>> +	if (len) {
>>>> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
>>>> +		*p |= mask_to_set;
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL(__bitmap_set);
>>>> +
>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
>>>> +{
>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>> +	const unsigned int size = start + len;
>>>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>>>> +
>>>> +	while (len - bits_to_clear >= 0) {
>>>> +		*p &= ~mask_to_clear;
>>>> +		len -= bits_to_clear;
>>>> +		bits_to_clear = BITS_PER_LONG;
>>>> +		mask_to_clear = ~0UL;
>>>> +		p++;
>>>> +	}
>>>> +	if (len) {
>>>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>>>> +		*p &= ~mask_to_clear;
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL(__bitmap_clear);
>>>
>>> Despite all the other EXPORT_SYMBOL() in this file, personally I
>>> would suggest to refrain from adding more. But I'm not going to
>>> insist (until such time that they all get cleaned up).
>>>
>>>> --- a/xen/include/asm-x86/bitops.h
>>>> +++ b/xen/include/asm-x86/bitops.h
>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>>>>   #define hweight16(x) generic_hweight16(x)
>>>>   #define hweight8(x) generic_hweight8(x)
>>>>   
>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>>>
>>> At first I thought - why for x86 only? Then I noticed Arm has an
>>> almost identical #define already. Which in turn made me look at
>>> Linux, where that #define lives in a common header. I think you
>>> want to move the Arm one. Or wait, no - Arm's isn't even
>>> compatible with the implementations of the functions you add.
>>> This definitely needs taking care of, perhaps by way of ignoring
>>> my request to go this route (as getting too involved).
>>
>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
>> used when the bitmap is mapped to an array of 32bit type elements.
>>
>> I could introduce BIT_LONG that would have the same definition on Arm
>> and x86, and then modify the imported functions to use it, but IMO the
>> right solution would be to change the Arm BIT_WORD macro to also use
>> BITS_PER_LONG (and adjust the callers).
> 
> So do I. Julien, Stefano?

BIT_WORD used to use BITS_PER_LONG but this was changed in commit:

commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a
Author: Ian Campbell <ian.campbell@citrix.com>
Date:   Thu May 8 16:13:55 2014 +0100

     xen: arm: bitops take unsigned int

     Xen bitmaps can be 4 rather than 8 byte aligned, so use the 
appropriate type.
     Otherwise the compiler can generate unaligned 8 byte accesses and 
cause traps.

     Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
     Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

On 64-bit Arm, while we allow unaligned access, the atomic operations 
still enforce alignment.

On 32-bit Arm, there are no unaligned access allowed. However,  the 
change of BIT_WORD is not a concern for 32-bit.

I haven't check whether we still have places where bitops are used with 
4 byte aligned memory. However, as the bitops take a void * in 
parameter, there are no promise on the alignment.

Therefore, we can't rewrite BIT_WORD without addressing the underlying 
issues. Introducing BIT_LONG is probably the easiest way at the moment.

However, our bitops really ought to specify the alignment in parameter 
to avoid such issues arising.

I would be in favor of using unsigned long *.

>> This seems quite far off, so if you don't mind I would rather have the
>> original v3 2/2 using set_bit:
>>
>> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html
> 
> As per my previous reply - yes, I'm okay with that, and yes,
> expecting this I've also kept your patches this way in my
> to-be-committed folder (pending Kevin's ack for patch 1).
> 
> Jan
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 08.02.2020 15:37, Julien Grall wrote:
> 
> 
> On 05/02/2020 13:27, Jan Beulich wrote:
>> On 05.02.2020 14:21, Roger Pau Monné wrote:
>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
>>>> On 04.02.2020 18:34, Roger Pau Monne wrote:
>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit
>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Thanks for going this route; two remarks / requests:
>>>>
>>>>> --- a/xen/common/bitmap.c
>>>>> +++ b/xen/common/bitmap.c
>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>>>>>   #endif
>>>>>   EXPORT_SYMBOL(__bitmap_weight);
>>>>>   
>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
>>>>> +{
>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>> +	const unsigned int size = start + len;
>>>>> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>>>>> +
>>>>> +	while (len - bits_to_set >= 0) {
>>>>> +		*p |= mask_to_set;
>>>>> +		len -= bits_to_set;
>>>>> +		bits_to_set = BITS_PER_LONG;
>>>>> +		mask_to_set = ~0UL;
>>>>> +		p++;
>>>>> +	}
>>>>> +	if (len) {
>>>>> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
>>>>> +		*p |= mask_to_set;
>>>>> +	}
>>>>> +}
>>>>> +EXPORT_SYMBOL(__bitmap_set);
>>>>> +
>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
>>>>> +{
>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>> +	const unsigned int size = start + len;
>>>>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>>>>> +
>>>>> +	while (len - bits_to_clear >= 0) {
>>>>> +		*p &= ~mask_to_clear;
>>>>> +		len -= bits_to_clear;
>>>>> +		bits_to_clear = BITS_PER_LONG;
>>>>> +		mask_to_clear = ~0UL;
>>>>> +		p++;
>>>>> +	}
>>>>> +	if (len) {
>>>>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>>>>> +		*p &= ~mask_to_clear;
>>>>> +	}
>>>>> +}
>>>>> +EXPORT_SYMBOL(__bitmap_clear);
>>>>
>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I
>>>> would suggest to refrain from adding more. But I'm not going to
>>>> insist (until such time that they all get cleaned up).
>>>>
>>>>> --- a/xen/include/asm-x86/bitops.h
>>>>> +++ b/xen/include/asm-x86/bitops.h
>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>>>>>   #define hweight16(x) generic_hweight16(x)
>>>>>   #define hweight8(x) generic_hweight8(x)
>>>>>   
>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>>>>
>>>> At first I thought - why for x86 only? Then I noticed Arm has an
>>>> almost identical #define already. Which in turn made me look at
>>>> Linux, where that #define lives in a common header. I think you
>>>> want to move the Arm one. Or wait, no - Arm's isn't even
>>>> compatible with the implementations of the functions you add.
>>>> This definitely needs taking care of, perhaps by way of ignoring
>>>> my request to go this route (as getting too involved).
>>>
>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
>>> used when the bitmap is mapped to an array of 32bit type elements.
>>>
>>> I could introduce BIT_LONG that would have the same definition on Arm
>>> and x86, and then modify the imported functions to use it, but IMO the
>>> right solution would be to change the Arm BIT_WORD macro to also use
>>> BITS_PER_LONG (and adjust the callers).
>>
>> So do I. Julien, Stefano?
> 
> BIT_WORD used to use BITS_PER_LONG but this was changed in commit:
> 
> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a
> Author: Ian Campbell <ian.campbell@citrix.com>
> Date:   Thu May 8 16:13:55 2014 +0100
> 
>      xen: arm: bitops take unsigned int
> 
>      Xen bitmaps can be 4 rather than 8 byte aligned, so use the 
> appropriate type.
>      Otherwise the compiler can generate unaligned 8 byte accesses and 
> cause traps.
> 
>      Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>      Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> On 64-bit Arm, while we allow unaligned access, the atomic operations 
> still enforce alignment.
> 
> On 32-bit Arm, there are no unaligned access allowed. However,  the 
> change of BIT_WORD is not a concern for 32-bit.
> 
> I haven't check whether we still have places where bitops are used with 
> 4 byte aligned memory. However, as the bitops take a void * in 
> parameter, there are no promise on the alignment.

I'm pretty sure for x86 the 32-bit guest compat code uses such, at
the very least.

> Therefore, we can't rewrite BIT_WORD without addressing the underlying 
> issues. Introducing BIT_LONG is probably the easiest way at the moment.

Which would make use (continue to) deviate from Linux'es meaning of
BIT_WORD().

> However, our bitops really ought to specify the alignment in parameter 
> to avoid such issues arising.
> 
> I would be in favor of using unsigned long *.

I don't think they should, as this complicates uses on non-64-bit
quantities. In fact I think bitops would better be permitted also
on sub-32-bit values. But anyway - x86 under the hood uses 32-bit
memory accesses too, in a number of cases. It's not obvious to me
why Arm64 couldn't do so as well, despite BIT_WORD() - for the
purposes of generic code - assuming "unsigned long" to be the base
"word".

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Julien Grall]

Hi Jan,

On 10/02/2020 08:43, Jan Beulich wrote:
> On 08.02.2020 15:37, Julien Grall wrote:
>>
>>
>> On 05/02/2020 13:27, Jan Beulich wrote:
>>> On 05.02.2020 14:21, Roger Pau Monné wrote:
>>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
>>>>> On 04.02.2020 18:34, Roger Pau Monne wrote:
>>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit
>>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
>>>>>>
>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>
>>>>> Thanks for going this route; two remarks / requests:
>>>>>
>>>>>> --- a/xen/common/bitmap.c
>>>>>> +++ b/xen/common/bitmap.c
>>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>>>>>>    #endif
>>>>>>    EXPORT_SYMBOL(__bitmap_weight);
>>>>>>    
>>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
>>>>>> +{
>>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>>> +	const unsigned int size = start + len;
>>>>>> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>>> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>>>>>> +
>>>>>> +	while (len - bits_to_set >= 0) {
>>>>>> +		*p |= mask_to_set;
>>>>>> +		len -= bits_to_set;
>>>>>> +		bits_to_set = BITS_PER_LONG;
>>>>>> +		mask_to_set = ~0UL;
>>>>>> +		p++;
>>>>>> +	}
>>>>>> +	if (len) {
>>>>>> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
>>>>>> +		*p |= mask_to_set;
>>>>>> +	}
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(__bitmap_set);
>>>>>> +
>>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
>>>>>> +{
>>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>>> +	const unsigned int size = start + len;
>>>>>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>>>>>> +
>>>>>> +	while (len - bits_to_clear >= 0) {
>>>>>> +		*p &= ~mask_to_clear;
>>>>>> +		len -= bits_to_clear;
>>>>>> +		bits_to_clear = BITS_PER_LONG;
>>>>>> +		mask_to_clear = ~0UL;
>>>>>> +		p++;
>>>>>> +	}
>>>>>> +	if (len) {
>>>>>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>>>>>> +		*p &= ~mask_to_clear;
>>>>>> +	}
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(__bitmap_clear);
>>>>>
>>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I
>>>>> would suggest to refrain from adding more. But I'm not going to
>>>>> insist (until such time that they all get cleaned up).
>>>>>
>>>>>> --- a/xen/include/asm-x86/bitops.h
>>>>>> +++ b/xen/include/asm-x86/bitops.h
>>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>>>>>>    #define hweight16(x) generic_hweight16(x)
>>>>>>    #define hweight8(x) generic_hweight8(x)
>>>>>>    
>>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>>>>>
>>>>> At first I thought - why for x86 only? Then I noticed Arm has an
>>>>> almost identical #define already. Which in turn made me look at
>>>>> Linux, where that #define lives in a common header. I think you
>>>>> want to move the Arm one. Or wait, no - Arm's isn't even
>>>>> compatible with the implementations of the functions you add.
>>>>> This definitely needs taking care of, perhaps by way of ignoring
>>>>> my request to go this route (as getting too involved).
>>>>
>>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
>>>> used when the bitmap is mapped to an array of 32bit type elements.
>>>>
>>>> I could introduce BIT_LONG that would have the same definition on Arm
>>>> and x86, and then modify the imported functions to use it, but IMO the
>>>> right solution would be to change the Arm BIT_WORD macro to also use
>>>> BITS_PER_LONG (and adjust the callers).
>>>
>>> So do I. Julien, Stefano?
>>
>> BIT_WORD used to use BITS_PER_LONG but this was changed in commit:
>>
>> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a
>> Author: Ian Campbell <ian.campbell@citrix.com>
>> Date:   Thu May 8 16:13:55 2014 +0100
>>
>>       xen: arm: bitops take unsigned int
>>
>>       Xen bitmaps can be 4 rather than 8 byte aligned, so use the
>> appropriate type.
>>       Otherwise the compiler can generate unaligned 8 byte accesses and
>> cause traps.
>>
>>       Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>       Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>> On 64-bit Arm, while we allow unaligned access, the atomic operations
>> still enforce alignment.
>>
>> On 32-bit Arm, there are no unaligned access allowed. However,  the
>> change of BIT_WORD is not a concern for 32-bit.
>>
>> I haven't check whether we still have places where bitops are used with
>> 4 byte aligned memory. However, as the bitops take a void * in
>> parameter, there are no promise on the alignment.
> 
> I'm pretty sure for x86 the 32-bit guest compat code uses such, at
> the very least.

I have spent some times looking at it and noticed, there are some in the 
common code (e.g scheduler, IRQ...).

> 
>> Therefore, we can't rewrite BIT_WORD without addressing the underlying
>> issues. Introducing BIT_LONG is probably the easiest way at the moment.
> 
> Which would make use (continue to) deviate from Linux'es meaning of
> BIT_WORD().

This would not be really the first time we deviate from Linux... We 
could possibly rename BIT_WORD to something different, but I don't have 
a good name, hence why I think BIT_LONG is the best solution so far.

> 
>> However, our bitops really ought to specify the alignment in parameter
>> to avoid such issues arising.
>>
>> I would be in favor of using unsigned long *.
> 
> I don't think they should, as this complicates uses on non-64-bit
> quantities. In fact I think bitops would better be permitted also
> on sub-32-bit values. But anyway - x86 under the hood uses 32-bit
> memory accesses too, in a number of cases. It's not obvious to me
> why Arm64 couldn't do so as well, despite BIT_WORD() - for the
> purposes of generic code - assuming "unsigned long" to be the base
> "word".

My point is we should avoid ot use void * and implicetly require an 
alignment (32-bit at the moment). This has resulted to numerous issues 
in the past on Arm. To be clear, I am not requesting to handle the void *.

Anyway, blindly updating BIT_WORD() is going to break Arm. So you either 
rename to current macro or create a new one.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 10.02.2020 10:20, Julien Grall wrote:
> Hi Jan,
> 
> On 10/02/2020 08:43, Jan Beulich wrote:
>> On 08.02.2020 15:37, Julien Grall wrote:
>>>
>>>
>>> On 05/02/2020 13:27, Jan Beulich wrote:
>>>> On 05.02.2020 14:21, Roger Pau Monné wrote:
>>>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
>>>>>> On 04.02.2020 18:34, Roger Pau Monne wrote:
>>>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit
>>>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
>>>>>>>
>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>>
>>>>>> Thanks for going this route; two remarks / requests:
>>>>>>
>>>>>>> --- a/xen/common/bitmap.c
>>>>>>> +++ b/xen/common/bitmap.c
>>>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>>>>>>>    #endif
>>>>>>>    EXPORT_SYMBOL(__bitmap_weight);
>>>>>>>    
>>>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
>>>>>>> +{
>>>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>>>> +	const unsigned int size = start + len;
>>>>>>> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>>>> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>>>>>>> +
>>>>>>> +	while (len - bits_to_set >= 0) {
>>>>>>> +		*p |= mask_to_set;
>>>>>>> +		len -= bits_to_set;
>>>>>>> +		bits_to_set = BITS_PER_LONG;
>>>>>>> +		mask_to_set = ~0UL;
>>>>>>> +		p++;
>>>>>>> +	}
>>>>>>> +	if (len) {
>>>>>>> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
>>>>>>> +		*p |= mask_to_set;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(__bitmap_set);
>>>>>>> +
>>>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
>>>>>>> +{
>>>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>>>> +	const unsigned int size = start + len;
>>>>>>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>>>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>>>>>>> +
>>>>>>> +	while (len - bits_to_clear >= 0) {
>>>>>>> +		*p &= ~mask_to_clear;
>>>>>>> +		len -= bits_to_clear;
>>>>>>> +		bits_to_clear = BITS_PER_LONG;
>>>>>>> +		mask_to_clear = ~0UL;
>>>>>>> +		p++;
>>>>>>> +	}
>>>>>>> +	if (len) {
>>>>>>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>>>>>>> +		*p &= ~mask_to_clear;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(__bitmap_clear);
>>>>>>
>>>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I
>>>>>> would suggest to refrain from adding more. But I'm not going to
>>>>>> insist (until such time that they all get cleaned up).
>>>>>>
>>>>>>> --- a/xen/include/asm-x86/bitops.h
>>>>>>> +++ b/xen/include/asm-x86/bitops.h
>>>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>>>>>>>    #define hweight16(x) generic_hweight16(x)
>>>>>>>    #define hweight8(x) generic_hweight8(x)
>>>>>>>    
>>>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>>>>>>
>>>>>> At first I thought - why for x86 only? Then I noticed Arm has an
>>>>>> almost identical #define already. Which in turn made me look at
>>>>>> Linux, where that #define lives in a common header. I think you
>>>>>> want to move the Arm one. Or wait, no - Arm's isn't even
>>>>>> compatible with the implementations of the functions you add.
>>>>>> This definitely needs taking care of, perhaps by way of ignoring
>>>>>> my request to go this route (as getting too involved).
>>>>>
>>>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
>>>>> used when the bitmap is mapped to an array of 32bit type elements.
>>>>>
>>>>> I could introduce BIT_LONG that would have the same definition on Arm
>>>>> and x86, and then modify the imported functions to use it, but IMO the
>>>>> right solution would be to change the Arm BIT_WORD macro to also use
>>>>> BITS_PER_LONG (and adjust the callers).
>>>>
>>>> So do I. Julien, Stefano?
>>>
>>> BIT_WORD used to use BITS_PER_LONG but this was changed in commit:
>>>
>>> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a
>>> Author: Ian Campbell <ian.campbell@citrix.com>
>>> Date:   Thu May 8 16:13:55 2014 +0100
>>>
>>>       xen: arm: bitops take unsigned int
>>>
>>>       Xen bitmaps can be 4 rather than 8 byte aligned, so use the
>>> appropriate type.
>>>       Otherwise the compiler can generate unaligned 8 byte accesses and
>>> cause traps.
>>>
>>>       Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>       Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>>> On 64-bit Arm, while we allow unaligned access, the atomic operations
>>> still enforce alignment.
>>>
>>> On 32-bit Arm, there are no unaligned access allowed. However,  the
>>> change of BIT_WORD is not a concern for 32-bit.
>>>
>>> I haven't check whether we still have places where bitops are used with
>>> 4 byte aligned memory. However, as the bitops take a void * in
>>> parameter, there are no promise on the alignment.
>>
>> I'm pretty sure for x86 the 32-bit guest compat code uses such, at
>> the very least.
> 
> I have spent some times looking at it and noticed, there are some in the 
> common code (e.g scheduler, IRQ...).
> 
>>
>>> Therefore, we can't rewrite BIT_WORD without addressing the underlying
>>> issues. Introducing BIT_LONG is probably the easiest way at the moment.
>>
>> Which would make use (continue to) deviate from Linux'es meaning of
>> BIT_WORD().
> 
> This would not be really the first time we deviate from Linux...

Of course. And there've been quite a few cases where I've argued
towards deviation. It's just that iirc you're one of those who
prefer less deviation, so I've been a little puzzled.

>>> However, our bitops really ought to specify the alignment in parameter
>>> to avoid such issues arising.
>>>
>>> I would be in favor of using unsigned long *.
>>
>> I don't think they should, as this complicates uses on non-64-bit
>> quantities. In fact I think bitops would better be permitted also
>> on sub-32-bit values. But anyway - x86 under the hood uses 32-bit
>> memory accesses too, in a number of cases. It's not obvious to me
>> why Arm64 couldn't do so as well, despite BIT_WORD() - for the
>> purposes of generic code - assuming "unsigned long" to be the base
>> "word".
> 
> My point is we should avoid ot use void * and implicetly require an 
> alignment (32-bit at the moment). This has resulted to numerous issues 
> in the past on Arm.

See how/why we have bitop_bad_size() on x86.

> To be clear, I am not requesting to handle the void *.
> 
> Anyway, blindly updating BIT_WORD() is going to break Arm. So you either 
> rename to current macro or create a new one.

That's understood. The question was rather what direction to go to
resolve the issue.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Julien Grall]

Hi Jan,

On 10/02/2020 09:31, Jan Beulich wrote:
> On 10.02.2020 10:20, Julien Grall wrote:
>> Hi Jan,
>>
>> On 10/02/2020 08:43, Jan Beulich wrote:
>>> On 08.02.2020 15:37, Julien Grall wrote:
>>>>
>>>>
>>>> On 05/02/2020 13:27, Jan Beulich wrote:
>>>>> On 05.02.2020 14:21, Roger Pau Monné wrote:
>>>>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
>>>>>>> On 04.02.2020 18:34, Roger Pau Monne wrote:
>>>>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit
>>>>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
>>>>>>>>
>>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>>>
>>>>>>> Thanks for going this route; two remarks / requests:
>>>>>>>
>>>>>>>> --- a/xen/common/bitmap.c
>>>>>>>> +++ b/xen/common/bitmap.c
>>>>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
>>>>>>>>     #endif
>>>>>>>>     EXPORT_SYMBOL(__bitmap_weight);
>>>>>>>>     
>>>>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len)
>>>>>>>> +{
>>>>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>>>>> +	const unsigned int size = start + len;
>>>>>>>> +	int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>>>>> +	unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>>>>>>>> +
>>>>>>>> +	while (len - bits_to_set >= 0) {
>>>>>>>> +		*p |= mask_to_set;
>>>>>>>> +		len -= bits_to_set;
>>>>>>>> +		bits_to_set = BITS_PER_LONG;
>>>>>>>> +		mask_to_set = ~0UL;
>>>>>>>> +		p++;
>>>>>>>> +	}
>>>>>>>> +	if (len) {
>>>>>>>> +		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
>>>>>>>> +		*p |= mask_to_set;
>>>>>>>> +	}
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(__bitmap_set);
>>>>>>>> +
>>>>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
>>>>>>>> +{
>>>>>>>> +	unsigned long *p = map + BIT_WORD(start);
>>>>>>>> +	const unsigned int size = start + len;
>>>>>>>> +	int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>>>>>>>> +	unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>>>>>>>> +
>>>>>>>> +	while (len - bits_to_clear >= 0) {
>>>>>>>> +		*p &= ~mask_to_clear;
>>>>>>>> +		len -= bits_to_clear;
>>>>>>>> +		bits_to_clear = BITS_PER_LONG;
>>>>>>>> +		mask_to_clear = ~0UL;
>>>>>>>> +		p++;
>>>>>>>> +	}
>>>>>>>> +	if (len) {
>>>>>>>> +		mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>>>>>>>> +		*p &= ~mask_to_clear;
>>>>>>>> +	}
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(__bitmap_clear);
>>>>>>>
>>>>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I
>>>>>>> would suggest to refrain from adding more. But I'm not going to
>>>>>>> insist (until such time that they all get cleaned up).
>>>>>>>
>>>>>>>> --- a/xen/include/asm-x86/bitops.h
>>>>>>>> +++ b/xen/include/asm-x86/bitops.h
>>>>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
>>>>>>>>     #define hweight16(x) generic_hweight16(x)
>>>>>>>>     #define hweight8(x) generic_hweight8(x)
>>>>>>>>     
>>>>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>>>>>>>
>>>>>>> At first I thought - why for x86 only? Then I noticed Arm has an
>>>>>>> almost identical #define already. Which in turn made me look at
>>>>>>> Linux, where that #define lives in a common header. I think you
>>>>>>> want to move the Arm one. Or wait, no - Arm's isn't even
>>>>>>> compatible with the implementations of the functions you add.
>>>>>>> This definitely needs taking care of, perhaps by way of ignoring
>>>>>>> my request to go this route (as getting too involved).
>>>>>>
>>>>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
>>>>>> used when the bitmap is mapped to an array of 32bit type elements.
>>>>>>
>>>>>> I could introduce BIT_LONG that would have the same definition on Arm
>>>>>> and x86, and then modify the imported functions to use it, but IMO the
>>>>>> right solution would be to change the Arm BIT_WORD macro to also use
>>>>>> BITS_PER_LONG (and adjust the callers).
>>>>>
>>>>> So do I. Julien, Stefano?
>>>>
>>>> BIT_WORD used to use BITS_PER_LONG but this was changed in commit:
>>>>
>>>> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a
>>>> Author: Ian Campbell <ian.campbell@citrix.com>
>>>> Date:   Thu May 8 16:13:55 2014 +0100
>>>>
>>>>        xen: arm: bitops take unsigned int
>>>>
>>>>        Xen bitmaps can be 4 rather than 8 byte aligned, so use the
>>>> appropriate type.
>>>>        Otherwise the compiler can generate unaligned 8 byte accesses and
>>>> cause traps.
>>>>
>>>>        Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>>        Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>
>>>> On 64-bit Arm, while we allow unaligned access, the atomic operations
>>>> still enforce alignment.
>>>>
>>>> On 32-bit Arm, there are no unaligned access allowed. However,  the
>>>> change of BIT_WORD is not a concern for 32-bit.
>>>>
>>>> I haven't check whether we still have places where bitops are used with
>>>> 4 byte aligned memory. However, as the bitops take a void * in
>>>> parameter, there are no promise on the alignment.
>>>
>>> I'm pretty sure for x86 the 32-bit guest compat code uses such, at
>>> the very least.
>>
>> I have spent some times looking at it and noticed, there are some in the
>> common code (e.g scheduler, IRQ...).
>>
>>>
>>>> Therefore, we can't rewrite BIT_WORD without addressing the underlying
>>>> issues. Introducing BIT_LONG is probably the easiest way at the moment.
>>>
>>> Which would make use (continue to) deviate from Linux'es meaning of
>>> BIT_WORD().
>>
>> This would not be really the first time we deviate from Linux...
> 
> Of course. And there've been quite a few cases where I've argued
> towards deviation. It's just that iirc you're one of those who
> prefer less deviation, so I've been a little puzzled.

I have been advocating deviation in a few cases ;).

> 
>>>> However, our bitops really ought to specify the alignment in parameter
>>>> to avoid such issues arising.
>>>>
>>>> I would be in favor of using unsigned long *.
>>>
>>> I don't think they should, as this complicates uses on non-64-bit
>>> quantities. In fact I think bitops would better be permitted also
>>> on sub-32-bit values. But anyway - x86 under the hood uses 32-bit
>>> memory accesses too, in a number of cases. It's not obvious to me
>>> why Arm64 couldn't do so as well, despite BIT_WORD() - for the
>>> purposes of generic code - assuming "unsigned long" to be the base
>>> "word".
>>
>> My point is we should avoid ot use void * and implicetly require an
>> alignment (32-bit at the moment). This has resulted to numerous issues
>> in the past on Arm.
> 
> See how/why we have bitop_bad_size() on x86.

Ah, I didn't spot the bitop_bad_size(). We could possibly use the same 
trick on Arm.

> 
>> To be clear, I am not requesting to handle the void *.
>>
>> Anyway, blindly updating BIT_WORD() is going to break Arm. So you either
>> rename to current macro or create a new one.
> 
> That's understood. The question was rather what direction to go to
> resolve the issue.

Please suggest a new name for BIT_WORD() and we can repurpose it. So 
far, I have no idea how to rename it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 10.02.2020 10:45, Julien Grall wrote:
> Please suggest a new name for BIT_WORD() and we can repurpose it. So 
> far, I have no idea how to rename it.

_BIT_WORD() if you/we were to accept the name space violation, or
BITMAP_WORD()?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Julien Grall]

On 10/02/2020 10:28, Jan Beulich wrote:
> On 10.02.2020 10:45, Julien Grall wrote:
>> Please suggest a new name for BIT_WORD() and we can repurpose it. So
>> far, I have no idea how to rename it.
> 
> _BIT_WORD() if you/we were to accept the name space violation, or
> BITMAP_WORD()?

BITMAP_WORD() is misleading as bitmap are using unsigned long. So my 
preference is _BIT_WORD().

Another alternative would be ATOMIC_WORD().

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 10.02.2020 12:00, Julien Grall wrote:
> On 10/02/2020 10:28, Jan Beulich wrote:
>> On 10.02.2020 10:45, Julien Grall wrote:
>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So
>>> far, I have no idea how to rename it.
>>
>> _BIT_WORD() if you/we were to accept the name space violation, or
>> BITMAP_WORD()?
> 
> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my 
> preference is _BIT_WORD().
> 
> Another alternative would be ATOMIC_WORD().

Except that there are also non-atomic bitmap operations, I don't really
care about the name as long as it's not BIT_WORD() (or anything else
that's likely to collide with other stuff).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Julien Grall]

Hi,

On 10/02/2020 11:59, Jan Beulich wrote:
> On 10.02.2020 12:00, Julien Grall wrote:
>> On 10/02/2020 10:28, Jan Beulich wrote:
>>> On 10.02.2020 10:45, Julien Grall wrote:
>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So
>>>> far, I have no idea how to rename it.
>>>
>>> _BIT_WORD() if you/we were to accept the name space violation, or
>>> BITMAP_WORD()?
>>
>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my
>> preference is _BIT_WORD().
>>
>> Another alternative would be ATOMIC_WORD().
> 
> Except that there are also non-atomic bitmap operations, I don't really
> care about the name as long as it's not BIT_WORD() (or anything else
> that's likely to collide with other stuff.

I am afraid we are disagreing on what is colliding with what here. The 
naming on Arm has been like that for the past few years. While this may 
not have been the best choice, this is your suggestion colliding with 
what is existing.

I am not entirely fussed about the namespace violation, although I think 
the name is potentially misleading. Yet, I would be happy to use 
_BIT_WORD() as this is the best of it so far.

While this is code falls under Arm maintainership, I am still happy to 
consider other naming. But at this point, you should be the one suggesting.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 10.02.2020 13:21, Julien Grall wrote:
> Hi,
> 
> On 10/02/2020 11:59, Jan Beulich wrote:
>> On 10.02.2020 12:00, Julien Grall wrote:
>>> On 10/02/2020 10:28, Jan Beulich wrote:
>>>> On 10.02.2020 10:45, Julien Grall wrote:
>>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So
>>>>> far, I have no idea how to rename it.
>>>>
>>>> _BIT_WORD() if you/we were to accept the name space violation, or
>>>> BITMAP_WORD()?
>>>
>>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my
>>> preference is _BIT_WORD().
>>>
>>> Another alternative would be ATOMIC_WORD().
>>
>> Except that there are also non-atomic bitmap operations, I don't really
>> care about the name as long as it's not BIT_WORD() (or anything else
>> that's likely to collide with other stuff.
> 
> I am afraid we are disagreing on what is colliding with what here. The 
> naming on Arm has been like that for the past few years. While this may 
> not have been the best choice, this is your suggestion colliding with 
> what is existing.

It is a plain import from Linux which has turned out impossible
because of the change that was done at some point to Arm code
which, I guess, also originally came from Linux. There's no new
naming I've been suggesting here at all.

> I am not entirely fussed about the namespace violation, although I think 
> the name is potentially misleading. Yet, I would be happy to use 
> _BIT_WORD() as this is the best of it so far.
> 
> While this is code falls under Arm maintainership, I am still happy to 
> consider other naming. But at this point, you should be the one suggesting.

BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Julien Grall]

On 10/02/2020 12:32, Jan Beulich wrote:
> On 10.02.2020 13:21, Julien Grall wrote:
>> Hi,
>>
>> On 10/02/2020 11:59, Jan Beulich wrote:
>>> On 10.02.2020 12:00, Julien Grall wrote:
>>>> On 10/02/2020 10:28, Jan Beulich wrote:
>>>>> On 10.02.2020 10:45, Julien Grall wrote:
>>>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So
>>>>>> far, I have no idea how to rename it.
>>>>>
>>>>> _BIT_WORD() if you/we were to accept the name space violation, or
>>>>> BITMAP_WORD()?
>>>>
>>>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my
>>>> preference is _BIT_WORD().
>>>>
>>>> Another alternative would be ATOMIC_WORD().
>>>
>>> Except that there are also non-atomic bitmap operations, I don't really
>>> care about the name as long as it's not BIT_WORD() (or anything else
>>> that's likely to collide with other stuff.
>>
>> I am afraid we are disagreing on what is colliding with what here. The
>> naming on Arm has been like that for the past few years. While this may
>> not have been the best choice, this is your suggestion colliding with
>> what is existing.
> 
> It is a plain import from Linux which has turned out impossible
> because of the change that was done at some point to Arm code
> which, I guess, also originally came from Linux. There's no new
> naming I've been suggesting here at all.

We never claimed we would be fully compatible with Linux and I don't 
think we could every claim it. Particularly, the bitop operations are 
different given Linux bitops are based on unsigned long.

The bitop did indeed came from Linux originally, however we had to adapt 
them because Linux Armv8 bitop was expecting 8-byte aligned. This does 
not hold on Xen.

> 
>> I am not entirely fussed about the namespace violation, although I think
>> the name is potentially misleading. Yet, I would be happy to use
>> _BIT_WORD() as this is the best of it so far.
>>
>> While this is code falls under Arm maintainership, I am still happy to
>> consider other naming. But at this point, you should be the one suggesting.
> 
> BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()?

BITOP_WORD().

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Roger Pau Monné]

On Mon, Feb 10, 2020 at 12:54:04PM +0000, Julien Grall wrote:
> 
> 
> On 10/02/2020 12:32, Jan Beulich wrote:
> > On 10.02.2020 13:21, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 10/02/2020 11:59, Jan Beulich wrote:
> > > > On 10.02.2020 12:00, Julien Grall wrote:
> > > > > On 10/02/2020 10:28, Jan Beulich wrote:
> > > > > > On 10.02.2020 10:45, Julien Grall wrote:
> > > > > > > Please suggest a new name for BIT_WORD() and we can repurpose it. So
> > > > > > > far, I have no idea how to rename it.
> > > > > > 
> > > > > > _BIT_WORD() if you/we were to accept the name space violation, or
> > > > > > BITMAP_WORD()?
> > > > > 
> > > > > BITMAP_WORD() is misleading as bitmap are using unsigned long. So my
> > > > > preference is _BIT_WORD().
> > > > > 
> > > > > Another alternative would be ATOMIC_WORD().
> > > > 
> > > > Except that there are also non-atomic bitmap operations, I don't really
> > > > care about the name as long as it's not BIT_WORD() (or anything else
> > > > that's likely to collide with other stuff.
> > > 
> > > I am afraid we are disagreing on what is colliding with what here. The
> > > naming on Arm has been like that for the past few years. While this may
> > > not have been the best choice, this is your suggestion colliding with
> > > what is existing.
> > 
> > It is a plain import from Linux which has turned out impossible
> > because of the change that was done at some point to Arm code
> > which, I guess, also originally came from Linux. There's no new
> > naming I've been suggesting here at all.
> 
> We never claimed we would be fully compatible with Linux and I don't think
> we could every claim it. Particularly, the bitop operations are different
> given Linux bitops are based on unsigned long.
> 
> The bitop did indeed came from Linux originally, however we had to adapt
> them because Linux Armv8 bitop was expecting 8-byte aligned. This does not
> hold on Xen.
> 
> > 
> > > I am not entirely fussed about the namespace violation, although I think
> > > the name is potentially misleading. Yet, I would be happy to use
> > > _BIT_WORD() as this is the best of it so far.
> > > 
> > > While this is code falls under Arm maintainership, I am still happy to
> > > consider other naming. But at this point, you should be the one suggesting.
> > 
> > BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()?
> 
> BITOP_WORD().

So I assume you would like me to import bitmap_{set/clear} and use it
in the x2APIC MSR patch?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich]

On 14.02.2020 11:40, Roger Pau Monné wrote:
> On Mon, Feb 10, 2020 at 12:54:04PM +0000, Julien Grall wrote:
>>
>>
>> On 10/02/2020 12:32, Jan Beulich wrote:
>>> On 10.02.2020 13:21, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 10/02/2020 11:59, Jan Beulich wrote:
>>>>> On 10.02.2020 12:00, Julien Grall wrote:
>>>>>> On 10/02/2020 10:28, Jan Beulich wrote:
>>>>>>> On 10.02.2020 10:45, Julien Grall wrote:
>>>>>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So
>>>>>>>> far, I have no idea how to rename it.
>>>>>>>
>>>>>>> _BIT_WORD() if you/we were to accept the name space violation, or
>>>>>>> BITMAP_WORD()?
>>>>>>
>>>>>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my
>>>>>> preference is _BIT_WORD().
>>>>>>
>>>>>> Another alternative would be ATOMIC_WORD().
>>>>>
>>>>> Except that there are also non-atomic bitmap operations, I don't really
>>>>> care about the name as long as it's not BIT_WORD() (or anything else
>>>>> that's likely to collide with other stuff.
>>>>
>>>> I am afraid we are disagreing on what is colliding with what here. The
>>>> naming on Arm has been like that for the past few years. While this may
>>>> not have been the best choice, this is your suggestion colliding with
>>>> what is existing.
>>>
>>> It is a plain import from Linux which has turned out impossible
>>> because of the change that was done at some point to Arm code
>>> which, I guess, also originally came from Linux. There's no new
>>> naming I've been suggesting here at all.
>>
>> We never claimed we would be fully compatible with Linux and I don't think
>> we could every claim it. Particularly, the bitop operations are different
>> given Linux bitops are based on unsigned long.
>>
>> The bitop did indeed came from Linux originally, however we had to adapt
>> them because Linux Armv8 bitop was expecting 8-byte aligned. This does not
>> hold on Xen.
>>
>>>
>>>> I am not entirely fussed about the namespace violation, although I think
>>>> the name is potentially misleading. Yet, I would be happy to use
>>>> _BIT_WORD() as this is the best of it so far.
>>>>
>>>> While this is code falls under Arm maintainership, I am still happy to
>>>> consider other naming. But at this point, you should be the one suggesting.
>>>
>>> BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()?
>>
>> BITOP_WORD().
> 
> So I assume you would like me to import bitmap_{set/clear} and use it
> in the x2APIC MSR patch?

Well, it's really up to you. I've put this as an item on my own todo
list already, assuming there may be more places where the two functions
might turn out useful.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 3/3] nvmx: always trap accesses to x2APIC MSRs

[Roger Pau Monne]

Nested VMX doesn't expose support for
SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE,
SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or
SECONDARY_EXEC_APIC_REGISTER_VIRT, and hence the x2APIC MSRs should
always be trapped in the nested guest MSR bitmap, or else a nested
guest could access the hardware x2APIC MSRs given certain conditions.

Accessing the hardware MSRs could be achieved by forcing the L0 Xen to
use SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE and
SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or
SECONDARY_EXEC_APIC_REGISTER_VIRT (if supported), and then creating a
L2 guest with a MSR bitmap that doesn't trap accesses to the x2APIC
MSR range. Then OR'ing both L0 and L1 MSR bitmaps would result in a
bitmap that doesn't trap certain x2APIC MSRs and a VMCS that doesn't
have SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE and
SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or
SECONDARY_EXEC_APIC_REGISTER_VIRT set either.

Fix this by making sure x2APIC MSRs are always trapped in the nested
MSR bitmap.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - Use bitmap_set.

Changes since v1:
 - New in this version (split from #1 patch).
 - Use non-locked set_bit.
---
 xen/arch/x86/hvm/vmx/vvmx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 46c51a95b9..56e0d884b8 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -596,6 +596,13 @@ static void update_msrbitmap(struct vcpu *v, uint32_t shadow_ctrl)
               v->arch.hvm.vmx.msr_bitmap->write_high,
               sizeof(msr_bitmap->write_high) * 8);
 
+    /*
+     * Nested VMX doesn't support any x2APIC hardware virtualization, so
+     * make sure all the x2APIC MSRs are trapped.
+     */
+    bitmap_set(msr_bitmap->read_low, MSR_X2APIC_FIRST, 0xff);
+    bitmap_set(msr_bitmap->write_low, MSR_X2APIC_FIRST, 0xff);
+
     unmap_domain_page(msr_bitmap);
 
     __vmwrite(MSR_BITMAP, page_to_maddr(nvmx->msr_merged));
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 3/3] nvmx: always trap accesses to x2APIC MSRs

[Jan Beulich]

On 04.02.2020 18:34, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -596,6 +596,13 @@ static void update_msrbitmap(struct vcpu *v, uint32_t shadow_ctrl)
>                v->arch.hvm.vmx.msr_bitmap->write_high,
>                sizeof(msr_bitmap->write_high) * 8);
>  
> +    /*
> +     * Nested VMX doesn't support any x2APIC hardware virtualization, so
> +     * make sure all the x2APIC MSRs are trapped.
> +     */
> +    bitmap_set(msr_bitmap->read_low, MSR_X2APIC_FIRST, 0xff);
> +    bitmap_set(msr_bitmap->write_low, MSR_X2APIC_FIRST, 0xff);

If you decide to address the Arm issue with these functions,
and hence don't go back to the v3 variant, then the last
arguments here need to be 0x100 afaict.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel