tools/libxc/include/xenctrl.h | 7 +++++++ tools/libxc/xc_altp2m.c | 24 ++++++++++++++++++++++ xen/arch/x86/hvm/hvm.c | 14 +++++++++++++ xen/arch/x86/hvm/vmx/vmx.c | 2 +- xen/arch/x86/mm/hap/hap.c | 15 ++++++++++++++ xen/arch/x86/mm/p2m-ept.c | 1 + xen/arch/x86/mm/p2m.c | 36 +++++++++++++++++++++++++++++++-- xen/include/asm-x86/domain.h | 1 + xen/include/asm-x86/p2m.h | 4 ++++ xen/include/public/hvm/hvm_op.h | 9 +++++++++ 10 files changed, 110 insertions(+), 3 deletions(-)
At this moment a guest can call vmfunc to change the altp2m view. This
should be limited in order to avoid any unwanted view switch.
The new xc_altp2m_set_visibility() solves this by making views invisible
to vmfunc.
This is done by having a separate arch.altp2m_working_eptp that is
populated and made invalid in the same places as altp2m_eptp. This is
written to EPTP_LIST_ADDR.
The views are made in/visible by marking them with INVALID_MFN or
copying them back from altp2m_eptp.
To have consistency the visibility also applies to
p2m_switch_domain_altp2m_by_id().
Note: If altp2m mode is set to mixed the guest is able to change the view
visibility and then call vmfunc.
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
Changes since V4:
- Move p2m specific things from hvm to p2m.c
- Add comment for altp2m_idx bounds check
- Add altp2m_list_lock/unlock().
Changes since V3:
- Change var name form altp2m_idx to idx to shorten line length
- Add bounds check for idx
- Update commit message
- Add comment in xenctrl.h.
Changes since V2:
- Drop hap_enabled() check
- Reduce the indentation depth in hvm.c
- Fix assignment indentation
- Drop pad2.
Changes since V1:
- Drop double view from title.
---
tools/libxc/include/xenctrl.h | 7 +++++++
tools/libxc/xc_altp2m.c | 24 ++++++++++++++++++++++
xen/arch/x86/hvm/hvm.c | 14 +++++++++++++
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
xen/arch/x86/mm/hap/hap.c | 15 ++++++++++++++
xen/arch/x86/mm/p2m-ept.c | 1 +
xen/arch/x86/mm/p2m.c | 36 +++++++++++++++++++++++++++++++--
xen/include/asm-x86/domain.h | 1 +
xen/include/asm-x86/p2m.h | 4 ++++
xen/include/public/hvm/hvm_op.h | 9 +++++++++
10 files changed, 110 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 99552a5f73..b26fccc989 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1943,6 +1943,13 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
xen_pfn_t new_gfn);
int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
uint32_t vcpuid, uint16_t *p2midx);
+/*
+ * Set view visibility for xc_altp2m_switch_to_view and vmfunc.
+ * Note: If altp2m mode is set to mixed the guest is able to change the view
+ * visibility and then call vmfunc.
+ */
+int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, bool visible);
/**
* Mem paging operations.
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 46fb725806..6987c9541f 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -410,3 +410,27 @@ int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
xc_hypercall_buffer_free(handle, arg);
return rc;
}
+
+int xc_altp2m_set_visibility(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, bool visible)
+{
+ int rc;
+
+ DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+ arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+ if ( arg == NULL )
+ return -1;
+
+ arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+ arg->cmd = HVMOP_altp2m_set_visibility;
+ arg->domain = domid;
+ arg->u.set_visibility.altp2m_idx = view_id;
+ arg->u.set_visibility.visible = visible;
+
+ rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+ HYPERCALL_BUFFER_AS_ARG(arg));
+
+ xc_hypercall_buffer_free(handle, arg);
+ return rc;
+}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a339b36a0d..25a65f6e25 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4563,6 +4563,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_get_mem_access:
case HVMOP_altp2m_change_gfn:
case HVMOP_altp2m_get_p2m_idx:
+ case HVMOP_altp2m_set_visibility:
break;
default:
@@ -4840,6 +4841,19 @@ static int do_altp2m_op(
break;
}
+ case HVMOP_altp2m_set_visibility:
+ {
+ uint16_t idx = a.u.set_visibility.altp2m_idx;
+
+ if ( a.u.set_visibility.pad )
+ rc = -EINVAL;
+ else if ( !altp2m_active(d) )
+ rc = -EOPNOTSUPP;
+ else
+ rc = p2m_set_altp2m_view_visibility(d, idx,
+ a.u.set_visibility.visible);
+ }
+
default:
ASSERT_UNREACHABLE();
}
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d265ed46ad..bb44ef39a1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2140,7 +2140,7 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
{
v->arch.hvm.vmx.secondary_exec_control |= mask;
__vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);
- __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_eptp));
+ __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_working_eptp));
if ( cpu_has_vmx_virt_exceptions )
{
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..5969ec8922 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
goto out;
}
+ if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
+ {
+ rv = -ENOMEM;
+ goto out;
+ }
+
for ( i = 0; i < MAX_EPTP; i++ )
+ {
d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+ d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
+ }
for ( i = 0; i < MAX_ALTP2M; i++ )
{
@@ -523,6 +532,12 @@ void hap_final_teardown(struct domain *d)
d->arch.altp2m_eptp = NULL;
}
+ if ( d->arch.altp2m_working_eptp )
+ {
+ free_xenheap_page(d->arch.altp2m_working_eptp);
+ d->arch.altp2m_working_eptp = NULL;
+ }
+
for ( i = 0; i < MAX_ALTP2M; i++ )
p2m_teardown(d->arch.altp2m_p2m[i]);
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index eb0f0edfef..6539ca619b 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1368,6 +1368,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
ept = &p2m->ept;
ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
+ d->arch.altp2m_working_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
}
unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c5f428d67c..27a812a7bb 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2515,6 +2515,7 @@ void p2m_flush_altp2m(struct domain *d)
{
p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+ d->arch.altp2m_working_eptp[i] = mfn_x(INVALID_MFN);
}
altp2m_list_unlock(d);
@@ -2634,7 +2635,9 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
{
p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
- mfn_x(INVALID_MFN);
+ mfn_x(INVALID_MFN);
+ d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
+ mfn_x(INVALID_MFN);
rc = 0;
}
}
@@ -2661,7 +2664,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
rc = -EINVAL;
altp2m_list_lock(d);
- if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
+ if ( d->arch.altp2m_working_eptp[idx] != mfn_x(INVALID_MFN) )
{
for_each_vcpu( d, v )
if ( idx != vcpu_altp2m(v).p2midx )
@@ -3145,6 +3148,35 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
return rc;
}
+
+int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
+ uint8_t visible)
+{
+ altp2m_list_lock(d);
+
+ /*
+ * Eptp index is correlated with altp2m index and should not exceed
+ * min(MAX_ALTP2M, MAX_EPTP).
+ */
+ if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
+ mfn_x(INVALID_MFN) )
+ {
+ altp2m_list_unlock(d);
+ return -EINVAL;
+ }
+
+ if ( visible )
+ d->arch.altp2m_working_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] =
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)];
+ else
+ d->arch.altp2m_working_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] =
+ mfn_x(INVALID_MFN);
+
+ altp2m_list_unlock(d);
+
+ return 0;
+}
#endif
/*
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 105adf96eb..800e12eae5 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -327,6 +327,7 @@ struct arch_domain
struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
mm_lock_t altp2m_list_lock;
uint64_t *altp2m_eptp;
+ uint64_t *altp2m_working_eptp;
#endif
/* NB. protected by d->event_lock and by irq_desc[irq].lock */
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 928a7c627a..015d509f7b 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -897,6 +897,10 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
mfn_t mfn, unsigned int page_order,
p2m_type_t p2mt, p2m_access_t p2ma);
+
+/* Set a specific p2m view visibility */
+int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
+ uint8_t visible);
#else
struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) {}
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index b599d3cbd0..870ec52060 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -318,6 +318,12 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
uint16_t altp2m_idx;
};
+struct xen_hvm_altp2m_set_visibility {
+ uint16_t altp2m_idx;
+ uint8_t visible;
+ uint8_t pad;
+};
+
struct xen_hvm_altp2m_op {
uint32_t version; /* HVMOP_ALTP2M_INTERFACE_VERSION */
uint32_t cmd;
@@ -350,6 +356,8 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_get_p2m_idx 14
/* Set the "Supress #VE" bit for a range of pages */
#define HVMOP_altp2m_set_suppress_ve_multi 15
+/* Set visibility for a given altp2m view */
+#define HVMOP_altp2m_set_visibility 16
domid_t domain;
uint16_t pad1;
uint32_t pad2;
@@ -367,6 +375,7 @@ struct xen_hvm_altp2m_op {
struct xen_hvm_altp2m_suppress_ve_multi suppress_ve_multi;
struct xen_hvm_altp2m_vcpu_disable_notify disable_notify;
struct xen_hvm_altp2m_get_vcpu_p2m_idx get_vcpu_p2m_idx;
+ struct xen_hvm_altp2m_set_visibility set_visibility;
uint8_t pad[64];
} u;
};
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
> break;
> }
>
> + case HVMOP_altp2m_set_visibility:
> + {
> + uint16_t idx = a.u.set_visibility.altp2m_idx;
Why a fixed width type (and even one inefficient to deal with)?
(One might even ask - why a local variable in the first place,
when it's used ...
> + if ( a.u.set_visibility.pad )
> + rc = -EINVAL;
> + else if ( !altp2m_active(d) )
> + rc = -EOPNOTSUPP;
> + else
> + rc = p2m_set_altp2m_view_visibility(d, idx,
> + a.u.set_visibility.visible);
... just once here.) The function takes "unsigned int" in any
event.
> @@ -3145,6 +3148,35 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
>
> return rc;
> }
> +
> +int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
> + uint8_t visible)
> +{
> + altp2m_list_lock(d);
> +
> + /*
> + * Eptp index is correlated with altp2m index and should not exceed
> + * min(MAX_ALTP2M, MAX_EPTP).
> + */
> + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> + mfn_x(INVALID_MFN) )
> + {
> + altp2m_list_unlock(d);
I think it would be nice if this went the normal function exit path.
Would be pretty simple to arrange for by introducing a local variable
holding the function return value.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 03.03.2020 11:30, Jan Beulich wrote:
> On 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
>> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
>> break;
>> }
>>
>> + case HVMOP_altp2m_set_visibility:
>> + {
>> + uint16_t idx = a.u.set_visibility.altp2m_idx;
>
> Why a fixed width type (and even one inefficient to deal with)?
> (One might even ask - why a local variable in the first place,
> when it's used ...
>
>> + if ( a.u.set_visibility.pad )
>> + rc = -EINVAL;
>> + else if ( !altp2m_active(d) )
>> + rc = -EOPNOTSUPP;
>> + else
>> + rc = p2m_set_altp2m_view_visibility(d, idx,
>> + a.u.set_visibility.visible);
>
> ... just once here.) The function takes "unsigned int" in any
> event.
Sure, I can have this idx dropped and use the value in the structure.
I had that in place to have line size smaller and the code easy to read.
>
>> @@ -3145,6 +3148,35 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
>>
>> return rc;
>> }
>> +
>> +int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
>> + uint8_t visible)
>> +{
>> + altp2m_list_lock(d);
>> +
>> + /*
>> + * Eptp index is correlated with altp2m index and should not exceed
>> + * min(MAX_ALTP2M, MAX_EPTP).
>> + */
>> + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
>> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>> + mfn_x(INVALID_MFN) )
>> + {
>> + altp2m_list_unlock(d);
>
> I think it would be nice if this went the normal function exit path.
> Would be pretty simple to arrange for by introducing a local variable
> holding the function return value.
>
I had the return here so as not to have boundary issues if the
altp2m_idx is wrong and then I have to manipulate altp2m_eptp[].
But sure, it can have a local rc var that is returned at the end of the
function and drop this unlock just to use a single one before the return.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 03.03.2020 10:43, Alexandru Stefan ISAILA wrote:
>
>
> On 03.03.2020 11:30, Jan Beulich wrote:
>> On 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
>>> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
>>> break;
>>> }
>>>
>>> + case HVMOP_altp2m_set_visibility:
>>> + {
>>> + uint16_t idx = a.u.set_visibility.altp2m_idx;
>>
>> Why a fixed width type (and even one inefficient to deal with)?
>> (One might even ask - why a local variable in the first place,
>> when it's used ...
>>
>>> + if ( a.u.set_visibility.pad )
>>> + rc = -EINVAL;
>>> + else if ( !altp2m_active(d) )
>>> + rc = -EOPNOTSUPP;
>>> + else
>>> + rc = p2m_set_altp2m_view_visibility(d, idx,
>>> + a.u.set_visibility.visible);
>>
>> ... just once here.) The function takes "unsigned int" in any
>> event.
>
> Sure, I can have this idx dropped and use the value in the structure.
> I had that in place to have line size smaller and the code easy to read.
Dropping the variable is secondary - if you prefer you keep it, so
be it. But if you keep it, its type should by in line with
./CODING_STYLE.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 03.03.2020 11:48, Jan Beulich wrote:
> On 03.03.2020 10:43, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 03.03.2020 11:30, Jan Beulich wrote:
>>> On 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
>>>> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
>>>> break;
>>>> }
>>>>
>>>> + case HVMOP_altp2m_set_visibility:
>>>> + {
>>>> + uint16_t idx = a.u.set_visibility.altp2m_idx;
>>>
>>> Why a fixed width type (and even one inefficient to deal with)?
>>> (One might even ask - why a local variable in the first place,
>>> when it's used ...
>>>
>>>> + if ( a.u.set_visibility.pad )
>>>> + rc = -EINVAL;
>>>> + else if ( !altp2m_active(d) )
>>>> + rc = -EOPNOTSUPP;
>>>> + else
>>>> + rc = p2m_set_altp2m_view_visibility(d, idx,
>>>> + a.u.set_visibility.visible);
>>>
>>> ... just once here.) The function takes "unsigned int" in any
>>> event.
>>
>> Sure, I can have this idx dropped and use the value in the structure.
>> I had that in place to have line size smaller and the code easy to read.
>
> Dropping the variable is secondary - if you prefer you keep it, so
> be it. But if you keep it, its type should by in line with
> ./CODING_STYLE.
>
Ah yes, you are right, I will change the type to unsigned int.
On the rc point you mentioned, I think it will be better to have a goto
label there and have the cleanup on "out:"
Thanks for the review,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 03.03.2020 10:59, Alexandru Stefan ISAILA wrote:
>
>
> On 03.03.2020 11:48, Jan Beulich wrote:
>> On 03.03.2020 10:43, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 03.03.2020 11:30, Jan Beulich wrote:
>>>> On 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
>>>>> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
>>>>> break;
>>>>> }
>>>>>
>>>>> + case HVMOP_altp2m_set_visibility:
>>>>> + {
>>>>> + uint16_t idx = a.u.set_visibility.altp2m_idx;
>>>>
>>>> Why a fixed width type (and even one inefficient to deal with)?
>>>> (One might even ask - why a local variable in the first place,
>>>> when it's used ...
>>>>
>>>>> + if ( a.u.set_visibility.pad )
>>>>> + rc = -EINVAL;
>>>>> + else if ( !altp2m_active(d) )
>>>>> + rc = -EOPNOTSUPP;
>>>>> + else
>>>>> + rc = p2m_set_altp2m_view_visibility(d, idx,
>>>>> + a.u.set_visibility.visible);
>>>>
>>>> ... just once here.) The function takes "unsigned int" in any
>>>> event.
>>>
>>> Sure, I can have this idx dropped and use the value in the structure.
>>> I had that in place to have line size smaller and the code easy to read.
>>
>> Dropping the variable is secondary - if you prefer you keep it, so
>> be it. But if you keep it, its type should by in line with
>> ./CODING_STYLE.
>>
>
> Ah yes, you are right, I will change the type to unsigned int.
>
>
> On the rc point you mentioned, I think it will be better to have a goto
> label there and have the cleanup on "out:"
My general position is that when error paths are complicated, "goto"
is acceptable. But when things can easily be done without "goto", its
use would better be avoided. In the case here all you need (afaict)
is a simple sequence of if/else-if/else.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.