Mailing List Archive: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}

[PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e()

Sep 16, 2020, 6:08 AM

Post #1 of 6 (73 views)

By passing the functions an MFN and flags, only a single instance of
each is needed; they were pretty large for being inline functions
anyway.

While moving the code, also adjust coding style and add const where
sensible / possible.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -903,6 +903,104 @@ int shadow_track_dirty_vram(struct domai
return rc;
}

+void shadow_vram_get_mfn(mfn_t mfn, unsigned int l1f,
+ mfn_t sl1mfn, const void *sl1e,
+ const struct domain *d)
+{
+ unsigned long gfn;
+ struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
+
+ ASSERT(is_hvm_domain(d));
+
+ if ( !dirty_vram /* tracking disabled? */ ||
+ !(l1f & _PAGE_RW) /* read-only mapping? */ ||
+ !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
+ return;
+
+ gfn = gfn_x(mfn_to_gfn(d, mfn));
+ /* Page sharing not supported on shadow PTs */
+ BUG_ON(SHARED_M2P(gfn));
+
+ if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
+ {
+ unsigned long i = gfn - dirty_vram->begin_pfn;
+ const struct page_info *page = mfn_to_page(mfn);
+
+ if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
+ /* Initial guest reference, record it */
+ dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn) |
+ PAGE_OFFSET(sl1e);
+ }
+}
+
+void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
+ mfn_t sl1mfn, const void *sl1e,
+ const struct domain *d)
+{
+ unsigned long gfn;
+ struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
+
+ ASSERT(is_hvm_domain(d));
+
+ if ( !dirty_vram /* tracking disabled? */ ||
+ !(l1f & _PAGE_RW) /* read-only mapping? */ ||
+ !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
+ return;
+
+ gfn = gfn_x(mfn_to_gfn(d, mfn));
+ /* Page sharing not supported on shadow PTs */
+ BUG_ON(SHARED_M2P(gfn));
+
+ if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
+ {
+ unsigned long i = gfn - dirty_vram->begin_pfn;
+ const struct page_info *page = mfn_to_page(mfn);
+ bool dirty = false;
+ paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
+
+ if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
+ {
+ /* Last reference */
+ if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
+ {
+ /* We didn't know it was that one, let's say it is dirty */
+ dirty = true;
+ }
+ else
+ {
+ ASSERT(dirty_vram->sl1ma[i] == sl1ma);
+ dirty_vram->sl1ma[i] = INVALID_PADDR;
+ if ( l1f & _PAGE_DIRTY )
+ dirty = true;
+ }
+ }
+ else
+ {
+ /* We had more than one reference, just consider the page dirty. */
+ dirty = true;
+ /* Check that it's not the one we recorded. */
+ if ( dirty_vram->sl1ma[i] == sl1ma )
+ {
+ /* Too bad, we remembered the wrong one... */
+ dirty_vram->sl1ma[i] = INVALID_PADDR;
+ }
+ else
+ {
+ /*
+ * Ok, our recorded sl1e is still pointing to this page, let's
+ * just hope it will remain.
+ */
+ }
+ }
+
+ if ( dirty )
+ {
+ dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
+ dirty_vram->last_dirty = NOW();
+ }
+ }
+}
+
/*
* Local variables:
* mode: C
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1047,107 +1047,6 @@ static int shadow_set_l2e(struct domain
return flags;
}

-static inline void shadow_vram_get_l1e(shadow_l1e_t new_sl1e,
- shadow_l1e_t *sl1e,
- mfn_t sl1mfn,
- struct domain *d)
-{
-#ifdef CONFIG_HVM
- mfn_t mfn = shadow_l1e_get_mfn(new_sl1e);
- int flags = shadow_l1e_get_flags(new_sl1e);
- unsigned long gfn;
- struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
-
- if ( !is_hvm_domain(d) || !dirty_vram /* tracking disabled? */
- || !(flags & _PAGE_RW) /* read-only mapping? */
- || !mfn_valid(mfn) ) /* mfn can be invalid in mmio_direct */
- return;
-
- gfn = gfn_x(mfn_to_gfn(d, mfn));
- /* Page sharing not supported on shadow PTs */
- BUG_ON(SHARED_M2P(gfn));
-
- if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
- {
- unsigned long i = gfn - dirty_vram->begin_pfn;
- struct page_info *page = mfn_to_page(mfn);
-
- if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
- /* Initial guest reference, record it */
- dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn)
- | ((unsigned long)sl1e & ~PAGE_MASK);
- }
-#endif
-}
-
-static inline void shadow_vram_put_l1e(shadow_l1e_t old_sl1e,
- shadow_l1e_t *sl1e,
- mfn_t sl1mfn,
- struct domain *d)
-{
-#ifdef CONFIG_HVM
- mfn_t mfn = shadow_l1e_get_mfn(old_sl1e);
- int flags = shadow_l1e_get_flags(old_sl1e);
- unsigned long gfn;
- struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
-
- if ( !is_hvm_domain(d) || !dirty_vram /* tracking disabled? */
- || !(flags & _PAGE_RW) /* read-only mapping? */
- || !mfn_valid(mfn) ) /* mfn can be invalid in mmio_direct */
- return;
-
- gfn = gfn_x(mfn_to_gfn(d, mfn));
- /* Page sharing not supported on shadow PTs */
- BUG_ON(SHARED_M2P(gfn));
-
- if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
- {
- unsigned long i = gfn - dirty_vram->begin_pfn;
- struct page_info *page = mfn_to_page(mfn);
- int dirty = 0;
- paddr_t sl1ma = mfn_to_maddr(sl1mfn)
- | ((unsigned long)sl1e & ~PAGE_MASK);
-
- if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
- {
- /* Last reference */
- if ( dirty_vram->sl1ma[i] == INVALID_PADDR ) {
- /* We didn't know it was that one, let's say it is dirty */
- dirty = 1;
- }
- else
- {
- ASSERT(dirty_vram->sl1ma[i] == sl1ma);
- dirty_vram->sl1ma[i] = INVALID_PADDR;
- if ( flags & _PAGE_DIRTY )
- dirty = 1;
- }
- }
- else
- {
- /* We had more than one reference, just consider the page dirty. */
- dirty = 1;
- /* Check that it's not the one we recorded. */
- if ( dirty_vram->sl1ma[i] == sl1ma )
- {
- /* Too bad, we remembered the wrong one... */
- dirty_vram->sl1ma[i] = INVALID_PADDR;
- }
- else
- {
- /* Ok, our recorded sl1e is still pointing to this page, let's
- * just hope it will remain. */
- }
- }
- if ( dirty )
- {
- dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
- dirty_vram->last_dirty = NOW();
- }
- }
-#endif
-}
-
static int shadow_set_l1e(struct domain *d,
shadow_l1e_t *sl1e,
shadow_l1e_t new_sl1e,
@@ -1156,6 +1055,7 @@ static int shadow_set_l1e(struct domain
{
int flags = 0;
shadow_l1e_t old_sl1e;
+ unsigned int old_sl1f;
#if SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC
mfn_t new_gmfn = shadow_l1e_get_mfn(new_sl1e);
#endif
@@ -1194,7 +1094,9 @@ static int shadow_set_l1e(struct domain
new_sl1e = shadow_l1e_flip_flags(new_sl1e, rc);
/* fall through */
case 0:
- shadow_vram_get_l1e(new_sl1e, sl1e, sl1mfn, d);
+ shadow_vram_get_mfn(shadow_l1e_get_mfn(new_sl1e),
+ shadow_l1e_get_flags(new_sl1e),
+ sl1mfn, sl1e, d);
break;
}
#undef PAGE_FLIPPABLE
@@ -1205,20 +1107,19 @@ static int shadow_set_l1e(struct domain
shadow_write_entries(sl1e, &new_sl1e, 1, sl1mfn);
flags |= SHADOW_SET_CHANGED;

- if ( (shadow_l1e_get_flags(old_sl1e) & _PAGE_PRESENT)
- && !sh_l1e_is_magic(old_sl1e) )
+ old_sl1f = shadow_l1e_get_flags(old_sl1e);
+ if ( (old_sl1f & _PAGE_PRESENT) && !sh_l1e_is_magic(old_sl1e) &&
+ shadow_mode_refcounts(d) )
{
/* We lost a reference to an old mfn. */
/* N.B. Unlike higher-level sets, never need an extra flush
* when writing an l1e. Because it points to the same guest frame
* as the guest l1e did, it's the guest's responsibility to
* trigger a flush later. */
- if ( shadow_mode_refcounts(d) )
- {
- shadow_vram_put_l1e(old_sl1e, sl1e, sl1mfn, d);
- shadow_put_page_from_l1e(old_sl1e, d);
- TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_SHADOW_L1_PUT_REF);
- }
+ shadow_vram_put_mfn(shadow_l1e_get_mfn(old_sl1e), old_sl1f,
+ sl1mfn, sl1e, d);
+ shadow_put_page_from_l1e(old_sl1e, d);
+ TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_SHADOW_L1_PUT_REF);
}
return flags;
}
@@ -1944,9 +1845,12 @@ void sh_destroy_l1_shadow(struct domain
/* Decrement refcounts of all the old entries */
mfn_t sl1mfn = smfn;
SHADOW_FOREACH_L1E(sl1mfn, sl1e, 0, 0, {
- if ( (shadow_l1e_get_flags(*sl1e) & _PAGE_PRESENT)
- && !sh_l1e_is_magic(*sl1e) ) {
- shadow_vram_put_l1e(*sl1e, sl1e, sl1mfn, d);
+ unsigned int sl1f = shadow_l1e_get_flags(*sl1e);
+
+ if ( (sl1f & _PAGE_PRESENT) && !sh_l1e_is_magic(*sl1e) )
+ {
+ shadow_vram_put_mfn(shadow_l1e_get_mfn(*sl1e), sl1f,
+ sl1mfn, sl1e, d);
shadow_put_page_from_l1e(*sl1e, d);
}
});
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -410,6 +410,14 @@ void shadow_update_paging_modes(struct v
* With user_only == 1, unhooks only the user-mode mappings. */
void shadow_unhook_mappings(struct domain *d, mfn_t smfn, int user_only);

+/* VRAM dirty tracking helpers. */
+void shadow_vram_get_mfn(mfn_t mfn, unsigned int l1f,
+ mfn_t sl1mfn, const void *sl1e,
+ const struct domain *d);
+void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
+ mfn_t sl1mfn, const void *sl1e,
+ const struct domain *d);
+
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
/* Allow a shadowed page to go out of sync */
int sh_unsync(struct vcpu *v, mfn_t gmfn);

Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e() [ In reply to ]

roger.pau at citrix

Oct 8, 2020, 8:15 AM

Post #2 of 6 (73 views)

Permalink

On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
> By passing the functions an MFN and flags, only a single instance of
^ a
> each is needed; they were pretty large for being inline functions
> anyway.
>
> While moving the code, also adjust coding style and add const where
> sensible / possible.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
>
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -903,6 +903,104 @@ int shadow_track_dirty_vram(struct domai
> return rc;
> }
>
> +void shadow_vram_get_mfn(mfn_t mfn, unsigned int l1f,
> + mfn_t sl1mfn, const void *sl1e,
> + const struct domain *d)
> +{
> + unsigned long gfn;
> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
> +
> + ASSERT(is_hvm_domain(d));
> +
> + if ( !dirty_vram /* tracking disabled? */ ||
> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
> + return;
> +
> + gfn = gfn_x(mfn_to_gfn(d, mfn));
> + /* Page sharing not supported on shadow PTs */
> + BUG_ON(SHARED_M2P(gfn));
> +
> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
> + {
> + unsigned long i = gfn - dirty_vram->begin_pfn;
> + const struct page_info *page = mfn_to_page(mfn);
> +
> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
> + /* Initial guest reference, record it */
> + dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn) |
> + PAGE_OFFSET(sl1e);
> + }
> +}
> +
> +void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
> + mfn_t sl1mfn, const void *sl1e,
> + const struct domain *d)
> +{
> + unsigned long gfn;
> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
> +
> + ASSERT(is_hvm_domain(d));
> +
> + if ( !dirty_vram /* tracking disabled? */ ||
> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
> + return;
> +
> + gfn = gfn_x(mfn_to_gfn(d, mfn));
> + /* Page sharing not supported on shadow PTs */
> + BUG_ON(SHARED_M2P(gfn));
> +
> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
> + {
> + unsigned long i = gfn - dirty_vram->begin_pfn;
> + const struct page_info *page = mfn_to_page(mfn);
> + bool dirty = false;
> + paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
> +
> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
> + {
> + /* Last reference */
> + if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
> + {
> + /* We didn't know it was that one, let's say it is dirty */
> + dirty = true;
> + }
> + else
> + {
> + ASSERT(dirty_vram->sl1ma[i] == sl1ma);
> + dirty_vram->sl1ma[i] = INVALID_PADDR;
> + if ( l1f & _PAGE_DIRTY )
> + dirty = true;
> + }
> + }
> + else
> + {
> + /* We had more than one reference, just consider the page dirty. */
> + dirty = true;
> + /* Check that it's not the one we recorded. */
> + if ( dirty_vram->sl1ma[i] == sl1ma )
> + {
> + /* Too bad, we remembered the wrong one... */
> + dirty_vram->sl1ma[i] = INVALID_PADDR;
> + }
> + else
> + {
> + /*
> + * Ok, our recorded sl1e is still pointing to this page, let's
> + * just hope it will remain.
> + */
> + }
> + }
> +
> + if ( dirty )
> + {
> + dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);

Could you use _set_bit here?

> + dirty_vram->last_dirty = NOW();
> + }
> + }
> +}
> +
> /*
> * Local variables:
> * mode: C
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -1047,107 +1047,6 @@ static int shadow_set_l2e(struct domain
> return flags;
> }
>
> -static inline void shadow_vram_get_l1e(shadow_l1e_t new_sl1e,
> - shadow_l1e_t *sl1e,
> - mfn_t sl1mfn,
> - struct domain *d)
> -{
> -#ifdef CONFIG_HVM
> - mfn_t mfn = shadow_l1e_get_mfn(new_sl1e);
> - int flags = shadow_l1e_get_flags(new_sl1e);
> - unsigned long gfn;
> - struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
> -
> - if ( !is_hvm_domain(d) || !dirty_vram /* tracking disabled? */
> - || !(flags & _PAGE_RW) /* read-only mapping? */
> - || !mfn_valid(mfn) ) /* mfn can be invalid in mmio_direct */
> - return;
> -
> - gfn = gfn_x(mfn_to_gfn(d, mfn));
> - /* Page sharing not supported on shadow PTs */
> - BUG_ON(SHARED_M2P(gfn));
> -
> - if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
> - {
> - unsigned long i = gfn - dirty_vram->begin_pfn;
> - struct page_info *page = mfn_to_page(mfn);
> -
> - if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
> - /* Initial guest reference, record it */
> - dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn)
> - | ((unsigned long)sl1e & ~PAGE_MASK);
> - }
> -#endif
> -}
> -
> -static inline void shadow_vram_put_l1e(shadow_l1e_t old_sl1e,
> - shadow_l1e_t *sl1e,
> - mfn_t sl1mfn,
> - struct domain *d)
> -{
> -#ifdef CONFIG_HVM
> - mfn_t mfn = shadow_l1e_get_mfn(old_sl1e);
> - int flags = shadow_l1e_get_flags(old_sl1e);
> - unsigned long gfn;
> - struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
> -
> - if ( !is_hvm_domain(d) || !dirty_vram /* tracking disabled? */
> - || !(flags & _PAGE_RW) /* read-only mapping? */
> - || !mfn_valid(mfn) ) /* mfn can be invalid in mmio_direct */
> - return;
> -
> - gfn = gfn_x(mfn_to_gfn(d, mfn));
> - /* Page sharing not supported on shadow PTs */
> - BUG_ON(SHARED_M2P(gfn));
> -
> - if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
> - {
> - unsigned long i = gfn - dirty_vram->begin_pfn;
> - struct page_info *page = mfn_to_page(mfn);
> - int dirty = 0;
> - paddr_t sl1ma = mfn_to_maddr(sl1mfn)
> - | ((unsigned long)sl1e & ~PAGE_MASK);
> -
> - if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
> - {
> - /* Last reference */
> - if ( dirty_vram->sl1ma[i] == INVALID_PADDR ) {
> - /* We didn't know it was that one, let's say it is dirty */
> - dirty = 1;
> - }
> - else
> - {
> - ASSERT(dirty_vram->sl1ma[i] == sl1ma);
> - dirty_vram->sl1ma[i] = INVALID_PADDR;
> - if ( flags & _PAGE_DIRTY )
> - dirty = 1;
> - }
> - }
> - else
> - {
> - /* We had more than one reference, just consider the page dirty. */
> - dirty = 1;
> - /* Check that it's not the one we recorded. */
> - if ( dirty_vram->sl1ma[i] == sl1ma )
> - {
> - /* Too bad, we remembered the wrong one... */
> - dirty_vram->sl1ma[i] = INVALID_PADDR;
> - }
> - else
> - {
> - /* Ok, our recorded sl1e is still pointing to this page, let's
> - * just hope it will remain. */
> - }
> - }
> - if ( dirty )
> - {
> - dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
> - dirty_vram->last_dirty = NOW();
> - }
> - }
> -#endif
> -}
> -
> static int shadow_set_l1e(struct domain *d,
> shadow_l1e_t *sl1e,
> shadow_l1e_t new_sl1e,
> @@ -1156,6 +1055,7 @@ static int shadow_set_l1e(struct domain
> {
> int flags = 0;
> shadow_l1e_t old_sl1e;
> + unsigned int old_sl1f;
> #if SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC
> mfn_t new_gmfn = shadow_l1e_get_mfn(new_sl1e);
> #endif
> @@ -1194,7 +1094,9 @@ static int shadow_set_l1e(struct domain
> new_sl1e = shadow_l1e_flip_flags(new_sl1e, rc);
> /* fall through */
> case 0:
> - shadow_vram_get_l1e(new_sl1e, sl1e, sl1mfn, d);
> + shadow_vram_get_mfn(shadow_l1e_get_mfn(new_sl1e),
> + shadow_l1e_get_flags(new_sl1e),
> + sl1mfn, sl1e, d);

As you have moved this function into a HVM build time file, don't you
need to guard this call, or alternatively provide a dummy handler for
!CONFIG_HVM in private.h?

Same for shadow_vram_put_mfn.

Thanks, Roger.

Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e() [ In reply to ]

andrew.cooper3 at citrix

Oct 8, 2020, 8:36 AM

Post #3 of 6 (73 views)

Permalink

On 08/10/2020 16:15, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
>> By passing the functions an MFN and flags, only a single instance of
> ^ a

'an' is correct.

an MFN
a Machine Frame Number

because the pronunciation changes. "an" precedes anything with a vowel
sound, not just vowels themselves. (Isn't English great...)

>> each is needed; they were pretty large for being inline functions
>> anyway.
>>
>> While moving the code, also adjust coding style and add const where
>> sensible / possible.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: New.
>>
>> --- a/xen/arch/x86/mm/shadow/hvm.c
>> +++ b/xen/arch/x86/mm/shadow/hvm.c
>> @@ -903,6 +903,104 @@ int shadow_track_dirty_vram(struct domai
>> return rc;
>> }
>>
>> +void shadow_vram_get_mfn(mfn_t mfn, unsigned int l1f,
>> + mfn_t sl1mfn, const void *sl1e,
>> + const struct domain *d)
>> +{
>> + unsigned long gfn;
>> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
>> +
>> + ASSERT(is_hvm_domain(d));
>> +
>> + if ( !dirty_vram /* tracking disabled? */ ||
>> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
>> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
>> + return;
>> +
>> + gfn = gfn_x(mfn_to_gfn(d, mfn));
>> + /* Page sharing not supported on shadow PTs */
>> + BUG_ON(SHARED_M2P(gfn));
>> +
>> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
>> + {
>> + unsigned long i = gfn - dirty_vram->begin_pfn;
>> + const struct page_info *page = mfn_to_page(mfn);
>> +
>> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
>> + /* Initial guest reference, record it */
>> + dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn) |
>> + PAGE_OFFSET(sl1e);
>> + }
>> +}
>> +
>> +void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
>> + mfn_t sl1mfn, const void *sl1e,
>> + const struct domain *d)
>> +{
>> + unsigned long gfn;
>> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
>> +
>> + ASSERT(is_hvm_domain(d));
>> +
>> + if ( !dirty_vram /* tracking disabled? */ ||
>> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
>> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
>> + return;
>> +
>> + gfn = gfn_x(mfn_to_gfn(d, mfn));
>> + /* Page sharing not supported on shadow PTs */
>> + BUG_ON(SHARED_M2P(gfn));
>> +
>> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
>> + {
>> + unsigned long i = gfn - dirty_vram->begin_pfn;
>> + const struct page_info *page = mfn_to_page(mfn);
>> + bool dirty = false;
>> + paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
>> +
>> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
>> + {
>> + /* Last reference */
>> + if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
>> + {
>> + /* We didn't know it was that one, let's say it is dirty */
>> + dirty = true;
>> + }
>> + else
>> + {
>> + ASSERT(dirty_vram->sl1ma[i] == sl1ma);
>> + dirty_vram->sl1ma[i] = INVALID_PADDR;
>> + if ( l1f & _PAGE_DIRTY )
>> + dirty = true;
>> + }
>> + }
>> + else
>> + {
>> + /* We had more than one reference, just consider the page dirty. */
>> + dirty = true;
>> + /* Check that it's not the one we recorded. */
>> + if ( dirty_vram->sl1ma[i] == sl1ma )
>> + {
>> + /* Too bad, we remembered the wrong one... */
>> + dirty_vram->sl1ma[i] = INVALID_PADDR;
>> + }
>> + else
>> + {
>> + /*
>> + * Ok, our recorded sl1e is still pointing to this page, let's
>> + * just hope it will remain.
>> + */
>> + }
>> + }
>> +
>> + if ( dirty )
>> + {
>> + dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
> Could you use _set_bit here?

__set_bit() uses 4-byte accesses. This uses 1-byte accesses.

Last I checked, there is a boundary issue at the end of the dirty_bitmap.

Both Julien and I have considered changing our bit infrastructure to use
byte accesses, which would make them more generally useful.

~Andrew

Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e() [ In reply to ]

roger.pau at citrix

Oct 10, 2020, 12:45 AM

Post #4 of 6 (73 views)

Permalink

On Thu, Oct 08, 2020 at 04:36:47PM +0100, Andrew Cooper wrote:
> On 08/10/2020 16:15, Roger Pau Monné wrote:
> > On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
> >> By passing the functions an MFN and flags, only a single instance of
> > ^ a
>
> 'an' is correct.
>
> an MFN
> a Machine Frame Number
>
> because the pronunciation changes. "an" precedes anything with a vowel
> sound, not just vowels themselves. (Isn't English great...)

Oh great, I think I've been misspelling this myself for a long time.

> >> each is needed; they were pretty large for being inline functions
> >> anyway.
> >>
> >> While moving the code, also adjust coding style and add const where
> >> sensible / possible.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> v2: New.
> >>
> >> --- a/xen/arch/x86/mm/shadow/hvm.c
> >> +++ b/xen/arch/x86/mm/shadow/hvm.c
> >> @@ -903,6 +903,104 @@ int shadow_track_dirty_vram(struct domai
> >> return rc;
> >> }
> >>
> >> +void shadow_vram_get_mfn(mfn_t mfn, unsigned int l1f,
> >> + mfn_t sl1mfn, const void *sl1e,
> >> + const struct domain *d)
> >> +{
> >> + unsigned long gfn;
> >> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
> >> +
> >> + ASSERT(is_hvm_domain(d));
> >> +
> >> + if ( !dirty_vram /* tracking disabled? */ ||
> >> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
> >> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
> >> + return;
> >> +
> >> + gfn = gfn_x(mfn_to_gfn(d, mfn));
> >> + /* Page sharing not supported on shadow PTs */
> >> + BUG_ON(SHARED_M2P(gfn));
> >> +
> >> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
> >> + {
> >> + unsigned long i = gfn - dirty_vram->begin_pfn;
> >> + const struct page_info *page = mfn_to_page(mfn);
> >> +
> >> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
> >> + /* Initial guest reference, record it */
> >> + dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn) |
> >> + PAGE_OFFSET(sl1e);
> >> + }
> >> +}
> >> +
> >> +void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
> >> + mfn_t sl1mfn, const void *sl1e,
> >> + const struct domain *d)
> >> +{
> >> + unsigned long gfn;
> >> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
> >> +
> >> + ASSERT(is_hvm_domain(d));
> >> +
> >> + if ( !dirty_vram /* tracking disabled? */ ||
> >> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
> >> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
> >> + return;
> >> +
> >> + gfn = gfn_x(mfn_to_gfn(d, mfn));
> >> + /* Page sharing not supported on shadow PTs */
> >> + BUG_ON(SHARED_M2P(gfn));
> >> +
> >> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
> >> + {
> >> + unsigned long i = gfn - dirty_vram->begin_pfn;
> >> + const struct page_info *page = mfn_to_page(mfn);
> >> + bool dirty = false;
> >> + paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
> >> +
> >> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
> >> + {
> >> + /* Last reference */
> >> + if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
> >> + {
> >> + /* We didn't know it was that one, let's say it is dirty */
> >> + dirty = true;
> >> + }
> >> + else
> >> + {
> >> + ASSERT(dirty_vram->sl1ma[i] == sl1ma);
> >> + dirty_vram->sl1ma[i] = INVALID_PADDR;
> >> + if ( l1f & _PAGE_DIRTY )
> >> + dirty = true;
> >> + }
> >> + }
> >> + else
> >> + {
> >> + /* We had more than one reference, just consider the page dirty. */
> >> + dirty = true;
> >> + /* Check that it's not the one we recorded. */
> >> + if ( dirty_vram->sl1ma[i] == sl1ma )
> >> + {
> >> + /* Too bad, we remembered the wrong one... */
> >> + dirty_vram->sl1ma[i] = INVALID_PADDR;
> >> + }
> >> + else
> >> + {
> >> + /*
> >> + * Ok, our recorded sl1e is still pointing to this page, let's
> >> + * just hope it will remain.
> >> + */
> >> + }
> >> + }
> >> +
> >> + if ( dirty )
> >> + {
> >> + dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
> > Could you use _set_bit here?
>
> __set_bit() uses 4-byte accesses. This uses 1-byte accesses.

Right, this is allocated using alloc directly, not the bitmap helper,
and the size if rounded to byte level, not unsigned int.

> Last I checked, there is a boundary issue at the end of the dirty_bitmap.
>
> Both Julien and I have considered changing our bit infrastructure to use
> byte accesses, which would make them more generally useful.

Does indeed seem useful to me, as we could safely expand the usage of
the bitmap ops without risking introducing bugs.

Thanks, Roger.

Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e() [ In reply to ]

jbeulich at suse

Oct 13, 2020, 3:29 AM

Post #5 of 6 (73 views)

Permalink

On 08.10.2020 17:15, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
>> +void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
>> + mfn_t sl1mfn, const void *sl1e,
>> + const struct domain *d)
>> +{
>> + unsigned long gfn;
>> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
>> +
>> + ASSERT(is_hvm_domain(d));
>> +
>> + if ( !dirty_vram /* tracking disabled? */ ||
>> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
>> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
>> + return;
>> +
>> + gfn = gfn_x(mfn_to_gfn(d, mfn));
>> + /* Page sharing not supported on shadow PTs */
>> + BUG_ON(SHARED_M2P(gfn));
>> +
>> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
>> + {
>> + unsigned long i = gfn - dirty_vram->begin_pfn;
>> + const struct page_info *page = mfn_to_page(mfn);
>> + bool dirty = false;
>> + paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
>> +
>> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
>> + {
>> + /* Last reference */
>> + if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
>> + {
>> + /* We didn't know it was that one, let's say it is dirty */
>> + dirty = true;
>> + }
>> + else
>> + {
>> + ASSERT(dirty_vram->sl1ma[i] == sl1ma);
>> + dirty_vram->sl1ma[i] = INVALID_PADDR;
>> + if ( l1f & _PAGE_DIRTY )
>> + dirty = true;
>> + }
>> + }
>> + else
>> + {
>> + /* We had more than one reference, just consider the page dirty. */
>> + dirty = true;
>> + /* Check that it's not the one we recorded. */
>> + if ( dirty_vram->sl1ma[i] == sl1ma )
>> + {
>> + /* Too bad, we remembered the wrong one... */
>> + dirty_vram->sl1ma[i] = INVALID_PADDR;
>> + }
>> + else
>> + {
>> + /*
>> + * Ok, our recorded sl1e is still pointing to this page, let's
>> + * just hope it will remain.
>> + */
>> + }
>> + }
>> +
>> + if ( dirty )
>> + {
>> + dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
>
> Could you use _set_bit here?

In addition to what Andrew has said - this would be a non cosmetic
change, which I wouldn't want to do in a patch merely moving this
code.

>> @@ -1194,7 +1094,9 @@ static int shadow_set_l1e(struct domain
>> new_sl1e = shadow_l1e_flip_flags(new_sl1e, rc);
>> /* fall through */
>> case 0:
>> - shadow_vram_get_l1e(new_sl1e, sl1e, sl1mfn, d);
>> + shadow_vram_get_mfn(shadow_l1e_get_mfn(new_sl1e),
>> + shadow_l1e_get_flags(new_sl1e),
>> + sl1mfn, sl1e, d);
>
> As you have moved this function into a HVM build time file, don't you
> need to guard this call, or alternatively provide a dummy handler for
> !CONFIG_HVM in private.h?
>
> Same for shadow_vram_put_mfn.

All uses are inside conditionals using shadow_mode_refcounts(), i.e.
the compiler's DCE pass will eliminate the calls. All we need are
declarations to be in scope.

Jan

Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e() [ In reply to ]

jbeulich at suse

Oct 13, 2020, 6:00 AM

Post #6 of 6 (73 views)

Permalink

On 10.10.2020 09:45, Roger Pau Monné wrote:
> On Thu, Oct 08, 2020 at 04:36:47PM +0100, Andrew Cooper wrote:
>> On 08/10/2020 16:15, Roger Pau Monné wrote:
>>> On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
>>>> +void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
>>>> + mfn_t sl1mfn, const void *sl1e,
>>>> + const struct domain *d)
>>>> +{
>>>> + unsigned long gfn;
>>>> + struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
>>>> +
>>>> + ASSERT(is_hvm_domain(d));
>>>> +
>>>> + if ( !dirty_vram /* tracking disabled? */ ||
>>>> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
>>>> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
>>>> + return;
>>>> +
>>>> + gfn = gfn_x(mfn_to_gfn(d, mfn));
>>>> + /* Page sharing not supported on shadow PTs */
>>>> + BUG_ON(SHARED_M2P(gfn));
>>>> +
>>>> + if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
>>>> + {
>>>> + unsigned long i = gfn - dirty_vram->begin_pfn;
>>>> + const struct page_info *page = mfn_to_page(mfn);
>>>> + bool dirty = false;
>>>> + paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
>>>> +
>>>> + if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
>>>> + {
>>>> + /* Last reference */
>>>> + if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
>>>> + {
>>>> + /* We didn't know it was that one, let's say it is dirty */
>>>> + dirty = true;
>>>> + }
>>>> + else
>>>> + {
>>>> + ASSERT(dirty_vram->sl1ma[i] == sl1ma);
>>>> + dirty_vram->sl1ma[i] = INVALID_PADDR;
>>>> + if ( l1f & _PAGE_DIRTY )
>>>> + dirty = true;
>>>> + }
>>>> + }
>>>> + else
>>>> + {
>>>> + /* We had more than one reference, just consider the page dirty. */
>>>> + dirty = true;
>>>> + /* Check that it's not the one we recorded. */
>>>> + if ( dirty_vram->sl1ma[i] == sl1ma )
>>>> + {
>>>> + /* Too bad, we remembered the wrong one... */
>>>> + dirty_vram->sl1ma[i] = INVALID_PADDR;
>>>> + }
>>>> + else
>>>> + {
>>>> + /*
>>>> + * Ok, our recorded sl1e is still pointing to this page, let's
>>>> + * just hope it will remain.
>>>> + */
>>>> + }
>>>> + }
>>>> +
>>>> + if ( dirty )
>>>> + {
>>>> + dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
>>> Could you use _set_bit here?
>>
>> __set_bit() uses 4-byte accesses. This uses 1-byte accesses.
>
> Right, this is allocated using alloc directly, not the bitmap helper,
> and the size if rounded to byte level, not unsigned int.
>
>> Last I checked, there is a boundary issue at the end of the dirty_bitmap.
>>
>> Both Julien and I have considered changing our bit infrastructure to use
>> byte accesses, which would make them more generally useful.
>
> Does indeed seem useful to me, as we could safely expand the usage of
> the bitmap ops without risking introducing bugs.

Aren't there architectures being handicapped when it comes to sub-word
accesses? At least common code may better not make assumptions towards
more fine grained accesses ...

As to x86, couldn't we make the macros evaluate alignof(*(addr)) and
choose byte-based accesses when it's less than 4?

Jan