Re: [PATCH 16/16] x86/P2M: the majority for struct p2m_domain's fields are HVM-only
On 14.02.2022 17:07, Jan Beulich wrote:
> On 14.02.2022 16:51, George Dunlap wrote:
>>> On Jul 5, 2021, at 5:15 PM, Jan Beulich wrote:
>>>
>>> ..., as are the majority of the locks involved. Conditionalize things
>>> accordingly.
>>>
>>> Also adjust the ioreq field's indentation at this occasion.
>>>
>>> Signed-off-by: Jan Beulich
>>
>> Reviewed-by: George Dunlap
>
> Thanks.
>
>> With one question…
>>
>>> @@ -905,10 +917,10 @@ int p2m_altp2m_propagate_change(struct d
>>> /* Set a specific p2m view visibility */
>>> int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
>>>uint8_t visible);
>>> -#else
>>> +#else /* CONFIG_HVM */
>>> struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
>>> static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) {}
>>> -#endif
>>> +#endif /* CONFIG_HVM */
>>
>> This is relatively minor, but what’s the normal for how to label #else
>> macros here? Wouldn’t you normally see “#endif /* CONFIG_HVM */“ and think
>> that the immediately preceding lines are compiled only if CONFIG_HVM is
>> defined? I.e., would this be more accurate to write “!CONFIG_HVM” here?
>>
>> I realize in this case it’s not a big deal since the #else is just three
>> lines above it, but since you took the time to add the comment in there, it
>> seems like it’s worth the time to have a quick think about whether that’s
>> the right thing to do.
>
> Hmm, yes, let me make this !CONFIG_HVM. I think we're not really
> consistent with this, but I agree it's more natural like you say.
Coming to write a similar construct elsewhere, I've realized this is
odd. Looking through include/asm/, the model generally used is
#ifdef CONFIG_xyz
#else /* !CONFIG_xyz */
#endif /* CONFIG_xyz */
That's what I'll switch to here then as well.
Jan
Re: [PATCH 16/16] x86/P2M: the majority for struct p2m_domain's fields are HVM-only
On 14.02.2022 16:51, George Dunlap wrote:
>
>
>> On Jul 5, 2021, at 5:15 PM, Jan Beulich wrote:
>>
>> ..., as are the majority of the locks involved. Conditionalize things
>> accordingly.
>>
>> Also adjust the ioreq field's indentation at this occasion.
>>
>> Signed-off-by: Jan Beulich
>
> Reviewed-by: George Dunlap
Thanks.
> With one question…
>
>> @@ -905,10 +917,10 @@ int p2m_altp2m_propagate_change(struct d
>> /* Set a specific p2m view visibility */
>> int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
>>uint8_t visible);
>> -#else
>> +#else /* CONFIG_HVM */
>> struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
>> static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) {}
>> -#endif
>> +#endif /* CONFIG_HVM */
>
> This is relatively minor, but what’s the normal for how to label #else macros
> here? Wouldn’t you normally see “#endif /* CONFIG_HVM */“ and think that the
> immediately preceding lines are compiled only if CONFIG_HVM is defined?
> I.e., would this be more accurate to write “!CONFIG_HVM” here?
>
> I realize in this case it’s not a big deal since the #else is just three
> lines above it, but since you took the time to add the comment in there, it
> seems like it’s worth the time to have a quick think about whether that’s the
> right thing to do.
Hmm, yes, let me make this !CONFIG_HVM. I think we're not really
consistent with this, but I agree it's more natural like you say.
Jan
Re: [PATCH 16/16] x86/P2M: the majority for struct p2m_domain's fields are HVM-only
> On Jul 5, 2021, at 5:15 PM, Jan Beulich wrote:
>
> ..., as are the majority of the locks involved. Conditionalize things
> accordingly.
>
> Also adjust the ioreq field's indentation at this occasion.
>
> Signed-off-by: Jan Beulich
Reviewed-by: George Dunlap
With one question…
> @@ -905,10 +917,10 @@ int p2m_altp2m_propagate_change(struct d
> /* Set a specific p2m view visibility */
> int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
>uint8_t visible);
> -#else
> +#else /* CONFIG_HVM */
> struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
> static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) {}
> -#endif
> +#endif /* CONFIG_HVM */
This is relatively minor, but what’s the normal for how to label #else macros
here? Wouldn’t you normally see “#endif /* CONFIG_HVM */“ and think that the
immediately preceding lines are compiled only if CONFIG_HVM is defined? I.e.,
would this be more accurate to write “!CONFIG_HVM” here?
I realize in this case it’s not a big deal since the #else is just three lines
above it, but since you took the time to add the comment in there, it seems
like it’s worth the time to have a quick think about whether that’s the right
thing to do.
-George
signature.asc
Description: Message signed with OpenPGP
Re: [PATCH 16/16] x86/P2M: the majority for struct p2m_domain's fields are HVM-only
On 05/07/2021 17:15, Jan Beulich wrote: ..., as are the majority of the locks involved. Conditionalize things accordingly. Also adjust the ioreq field's indentation at this occasion. Signed-off-by: Jan Beulich Reviewed-by: Paul Durrant
[PATCH 16/16] x86/P2M: the majority for struct p2m_domain's fields are HVM-only
..., as are the majority of the locks involved. Conditionalize things
accordingly.
Also adjust the ioreq field's indentation at this occasion.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -481,8 +481,11 @@ unsigned int page_get_ram_type(mfn_t mfn
unsigned long domain_get_maximum_gpfn(struct domain *d)
{
+#ifdef CONFIG_HVM
if ( is_hvm_domain(d) )
return p2m_get_hostp2m(d)->max_mapped_pfn;
+#endif
+
/* NB. PV guests specify nr_pfns rather than max_pfn so we adjust here. */
return (arch_get_max_pfn(d) ?: 1) - 1;
}
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -237,6 +237,8 @@ static inline void mm_enforce_order_unlo
* *
/
+#ifdef CONFIG_HVM
+
/* Nested P2M lock (per-domain)
*
* A per-domain lock that protects the mapping from nested-CR3 to
@@ -354,6 +356,8 @@ declare_mm_lock(pod)
#define pod_unlock(p) mm_unlock(&(p)->pod.lock)
#define pod_locked_by_me(p) mm_locked_by_me(&(p)->pod.lock)
+#endif /* CONFIG_HVM */
+
/* Page alloc lock (per-domain)
*
* This is an external lock, not represented by an mm_lock_t. However,
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -48,6 +48,8 @@
#undef virt_to_mfn
#define virt_to_mfn(v) _mfn(__virt_to_mfn(v))
+DEFINE_PERCPU_RWLOCK_GLOBAL(p2m_percpu_rwlock);
+
/* Turn on/off host superpage page table support for hap, default on. */
bool_t __initdata opt_hap_1gb = 1, __initdata opt_hap_2mb = 1;
boolean_param("hap_1gb", opt_hap_1gb);
--- a/xen/arch/x86/mm/p2m-basic.c
+++ b/xen/arch/x86/mm/p2m-basic.c
@@ -28,16 +28,15 @@
#include "mm-locks.h"
#include "p2m.h"
-DEFINE_PERCPU_RWLOCK_GLOBAL(p2m_percpu_rwlock);
-
/* Init the datastructures for later use by the p2m code */
static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
{
int ret = 0;
-mm_rwlock_init(>lock);
#ifdef CONFIG_HVM
+mm_rwlock_init(>lock);
INIT_PAGE_LIST_HEAD(>pages);
+spin_lock_init(>ioreq.lock);
#endif
p2m->domain = d;
@@ -55,8 +54,6 @@ static int p2m_initialise(struct domain
else
p2m_pt_init(p2m);
-spin_lock_init(>ioreq.lock);
-
return ret;
}
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -338,7 +338,7 @@ bool arch_iommu_use_permitted(const stru
return d == dom_io ||
(likely(!mem_sharing_enabled(d)) &&
likely(!mem_paging_enabled(d)) &&
-likely(!p2m_get_hostp2m(d)->global_logdirty));
+likely(!p2m_is_global_logdirty(d)));
}
/*
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -199,8 +199,10 @@ typedef enum {
/* Per-p2m-table state */
struct p2m_domain {
+#ifdef CONFIG_HVM
/* Lock that protects updates to the p2m */
mm_rwlock_t lock;
+#endif
/*
* Same as a domain's dirty_cpumask but limited to
@@ -220,13 +222,14 @@ struct p2m_domain {
*/
p2m_access_t default_access;
+#ifdef CONFIG_HVM
+
/* Host p2m: Log-dirty ranges registered for the domain. */
struct rangeset *logdirty_ranges;
/* Host p2m: Global log-dirty mode enabled for the domain. */
bool global_logdirty;
-#ifdef CONFIG_HVM
/* Translated domain: p2m mapping */
pagetable_tphys_table;
@@ -269,7 +272,6 @@ struct p2m_domain {
unsigned int level);
void (*write_p2m_entry_post)(struct p2m_domain *p2m,
unsigned int oflags);
-#endif
#if P2M_AUDIT
long (*audit_p2m)(struct p2m_domain *p2m);
#endif
@@ -304,7 +306,6 @@ struct p2m_domain {
unsigned long min_remapped_gfn;
unsigned long max_remapped_gfn;
-#ifdef CONFIG_HVM
/* Populate-on-demand variables
* All variables are protected with the pod lock. We cannot rely on
* the p2m lock if it's turned into a fine-grained lock.
@@ -361,27 +362,27 @@ struct p2m_domain {
* threaded on in LRU order.
*/
struct list_head np2m_list;
-#endif
union {
struct ept_data ept;
/* NPT-equivalent structure could be added here. */
};
- struct {
- spinlock_t lock;
- /*
- * ioreq server who's responsible for the emulation of
- * gfns with specific p2m type(for now, p2m_ioreq_server).
- */
- struct ioreq_server *server;
- /*
- * flags specifies whether read, write or both operations
- * are to be emulated by an ioreq server.
- */
- unsigned int flags;
- unsigned long entry_count;
- } ioreq;
+struct {
+spinlock_t lock;
+/*
+ * ioreq server who's responsible for the emulation of
+ * gfns with specific p2m type(for now,
