(Not so) large (anymore) parts of this series are to further isolate pieces which are needed for HVM only, and hence would better not be built with HVM=n. But there are also a few other items which I've noticed along the road, including the new in v2 4th patch. 1: mm: no-one passes a NULL domain to init_xen_l4_slots() 2: shadow: sh_update_linear_entries() is a no-op for PV 3: mm: monitor table is HVM-only 4: adjustments to guest handle treatment Jan
Drop the NULL checks - they've been introduced by commit 8d7b633ada
("x86/mm: Consolidate all Xen L4 slot writing into
init_xen_l4_slots()") for no apparent reason.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Adjust comment ahead of the function.
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1653,7 +1653,7 @@ static int promote_l3_table(struct page_
* This function must write all ROOT_PAGETABLE_PV_XEN_SLOTS, to clobber any
* values a guest may have left there from promote_l4_table().
*
- * l4t and l4mfn are mandatory, but l4mfn doesn't need to be the mfn under
+ * l4t, l4mfn, and d are mandatory, but l4mfn doesn't need to be the mfn under
* *l4t. All other parameters are optional and will either fill or zero the
* appropriate slots. Pagetables not shared with guests will gain the
* extended directmap.
@@ -1665,7 +1665,7 @@ void init_xen_l4_slots(l4_pgentry_t *l4t
* PV vcpus need a shortened directmap. HVM and Idle vcpus get the full
* directmap.
*/
- bool short_directmap = d && !paging_mode_external(d);
+ bool short_directmap = !paging_mode_external(d);
/* Slot 256: RO M2P (if applicable). */
l4t[l4_table_offset(RO_MPT_VIRT_START)] =
@@ -1686,10 +1686,9 @@ void init_xen_l4_slots(l4_pgentry_t *l4t
mfn_eq(sl4mfn, INVALID_MFN) ? l4e_empty() :
l4e_from_mfn(sl4mfn, __PAGE_HYPERVISOR_RW);
- /* Slot 260: Per-domain mappings (if applicable). */
+ /* Slot 260: Per-domain mappings. */
l4t[l4_table_offset(PERDOMAIN_VIRT_START)] =
- d ? l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW)
- : l4e_empty();
+ l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
/* Slot 261-: text/data/bss, RW M2P, vmap, frametable, directmap. */
#ifndef NDEBUG
On Tue, Apr 21, 2020 at 11:11:03AM +0200, Jan Beulich wrote:
> Drop the NULL checks - they've been introduced by commit 8d7b633ada
> ("x86/mm: Consolidate all Xen L4 slot writing into
> init_xen_l4_slots()") for no apparent reason.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks.
Andrew,
On 21.04.2020 18:40, Roger Pau Monné wrote:
> On Tue, Apr 21, 2020 at 11:11:03AM +0200, Jan Beulich wrote:
>> Drop the NULL checks - they've been introduced by commit 8d7b633ada
>> ("x86/mm: Consolidate all Xen L4 slot writing into
>> init_xen_l4_slots()") for no apparent reason.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
you weren't entirely happy with the change because of the
possible (or, as you state, necessary) need to undo this. I
still think in the current shape the NULL checks are
pointless and hence would better go away. Re-introducing them
(adjusted to whatever shape the function may be in by that
time) is not that big of a problem. May I ask that you
explicitly clarify whether you actively NAK the patch, accept
it going in with Roger's R-b, or would be willing to ack it?
Thanks, Jan
On 05/05/2020 07:31, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> Andrew,
>
> On 21.04.2020 18:40, Roger Pau Monné wrote:
>> On Tue, Apr 21, 2020 at 11:11:03AM +0200, Jan Beulich wrote:
>>> Drop the NULL checks - they've been introduced by commit 8d7b633ada
>>> ("x86/mm: Consolidate all Xen L4 slot writing into
>>> init_xen_l4_slots()") for no apparent reason.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> you weren't entirely happy with the change because of the
> possible (or, as you state, necessary) need to undo this. I
> still think in the current shape the NULL checks are
> pointless and hence would better go away. Re-introducing them
> (adjusted to whatever shape the function may be in by that
> time) is not that big of a problem. May I ask that you
> explicitly clarify whether you actively NAK the patch, accept
> it going in with Roger's R-b, or would be willing to ack it?
I'm not going to nack it, because that would be petty, but I still don't
think it is a useful use of your time to be making more work for someone
in the future to revert.
However, if you wish to take the patch with Roger's R-b, then please fix
the stale commit message, seeing as this is v2 and I explained exactly
why it was done like that.
~Andrew
On 07.05.2020 19:26, Andrew Cooper wrote:
> On 05/05/2020 07:31, Jan Beulich wrote:
>> On 21.04.2020 18:40, Roger Pau Monné wrote:
>>> On Tue, Apr 21, 2020 at 11:11:03AM +0200, Jan Beulich wrote:
>>>> Drop the NULL checks - they've been introduced by commit 8d7b633ada
>>>> ("x86/mm: Consolidate all Xen L4 slot writing into
>>>> init_xen_l4_slots()") for no apparent reason.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>> you weren't entirely happy with the change because of the
>> possible (or, as you state, necessary) need to undo this. I
>> still think in the current shape the NULL checks are
>> pointless and hence would better go away. Re-introducing them
>> (adjusted to whatever shape the function may be in by that
>> time) is not that big of a problem. May I ask that you
>> explicitly clarify whether you actively NAK the patch, accept
>> it going in with Roger's R-b, or would be willing to ack it?
>
> I'm not going to nack it, because that would be petty, but I still don't
> think it is a useful use of your time to be making more work for someone
> in the future to revert.
>
> However, if you wish to take the patch with Roger's R-b, then please fix
> the stale commit message, seeing as this is v2 and I explained exactly
> why it was done like that.
Is "... without giving a reason; I'm told this was done in anticipation
of the function potentially getting called with a NULL argument" any
better? I don't think the commit message here was stale, as said commit
indeed gives no explanation, yet all call sites pass non-NULL.
Jan
Consolidate the shadow_mode_external() in here: Check this once at the
start of the function.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v2: Delete stale part of comment.
---
Tim - I'm re-posting as I wasn't entirely sure whether you meant to drop
the entire PV part of the comment, or only the last two sentences.
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3680,20 +3680,7 @@ sh_update_linear_entries(struct vcpu *v)
{
struct domain *d = v->domain;
- /* Linear pagetables in PV guests
- * ------------------------------
- *
- * Guest linear pagetables, which map the guest pages, are at
- * LINEAR_PT_VIRT_START. Shadow linear pagetables, which map the
- * shadows, are at SH_LINEAR_PT_VIRT_START. Most of the time these
- * are set up at shadow creation time, but (of course!) the PAE case
- * is subtler. Normal linear mappings are made by having an entry
- * in the top-level table that points to itself (shadow linear) or
- * to the guest top-level table (guest linear). For PAE, to set up
- * a linear map requires us to copy the four top-level entries into
- * level-2 entries. That means that every time we change a PAE l3e,
- * we need to reflect the change into the copy.
- *
+ /*
* Linear pagetables in HVM guests
* -------------------------------
*
@@ -3711,34 +3698,30 @@ sh_update_linear_entries(struct vcpu *v)
*/
/* Don't try to update the monitor table if it doesn't exist */
- if ( shadow_mode_external(d)
- && pagetable_get_pfn(v->arch.monitor_table) == 0 )
+ if ( !shadow_mode_external(d) ||
+ pagetable_get_pfn(v->arch.monitor_table) == 0 )
return;
#if SHADOW_PAGING_LEVELS == 4
- /* For PV, one l4e points at the guest l4, one points at the shadow
- * l4. No maintenance required.
- * For HVM, just need to update the l4e that points to the shadow l4. */
+ /* For HVM, just need to update the l4e that points to the shadow l4. */
- if ( shadow_mode_external(d) )
+ /* Use the linear map if we can; otherwise make a new mapping */
+ if ( v == current )
{
- /* Use the linear map if we can; otherwise make a new mapping */
- if ( v == current )
- {
- __linear_l4_table[l4_linear_offset(SH_LINEAR_PT_VIRT_START)] =
- l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
- __PAGE_HYPERVISOR_RW);
- }
- else
- {
- l4_pgentry_t *ml4e;
- ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
- ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
- l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
- __PAGE_HYPERVISOR_RW);
- unmap_domain_page(ml4e);
- }
+ __linear_l4_table[l4_linear_offset(SH_LINEAR_PT_VIRT_START)] =
+ l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
+ __PAGE_HYPERVISOR_RW);
+ }
+ else
+ {
+ l4_pgentry_t *ml4e;
+
+ ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
+ ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
+ l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
+ __PAGE_HYPERVISOR_RW);
+ unmap_domain_page(ml4e);
}
#elif SHADOW_PAGING_LEVELS == 3
@@ -3752,7 +3735,6 @@ sh_update_linear_entries(struct vcpu *v)
* the shadows.
*/
- ASSERT(shadow_mode_external(d));
{
/* Install copies of the shadow l3es into the monitor l2 table
* that maps SH_LINEAR_PT_VIRT_START. */
@@ -3803,20 +3785,16 @@ sh_update_linear_entries(struct vcpu *v)
#error this should not happen
#endif
- if ( shadow_mode_external(d) )
- {
- /*
- * Having modified the linear pagetable mapping, flush local host TLBs.
- * This was not needed when vmenter/vmexit always had the side effect
- * of flushing host TLBs but, with ASIDs, it is possible to finish
- * this CR3 update, vmenter the guest, vmexit due to a page fault,
- * without an intervening host TLB flush. Then the page fault code
- * could use the linear pagetable to read a top-level shadow page
- * table entry. But, without this change, it would fetch the wrong
- * value due to a stale TLB.
- */
- flush_tlb_local();
- }
+ /*
+ * Having modified the linear pagetable mapping, flush local host TLBs.
+ * This was not needed when vmenter/vmexit always had the side effect of
+ * flushing host TLBs but, with ASIDs, it is possible to finish this CR3
+ * update, vmenter the guest, vmexit due to a page fault, without an
+ * intervening host TLB flush. Then the page fault code could use the
+ * linear pagetable to read a top-level shadow page table entry. But,
+ * without this change, it would fetch the wrong value due to a stale TLB.
+ */
+ flush_tlb_local();
}
At 11:11 +0200 on 21 Apr (1587467497), Jan Beulich wrote: > Consolidate the shadow_mode_external() in here: Check this once at the > start of the function. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Acked-by: Tim Deegan <tim@xen.org> > --- > v2: Delete stale part of comment. > --- > Tim - I'm re-posting as I wasn't entirely sure whether you meant to drop > the entire PV part of the comment, or only the last two sentences. Looks good, thanks! Acked-by: Tim Deegan <tim@xen.org>
Move the per-vCPU field to the HVM sub-structure.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -545,7 +545,7 @@ void write_ptbase(struct vcpu *v)
* Should be called after CR3 is updated.
*
* Uses values found in vcpu->arch.(guest_table and guest_table_user), and
- * for HVM guests, arch.monitor_table and hvm's guest CR3.
+ * for HVM guests, arch.hvm.monitor_table and hvm's guest CR3.
*
* Update ref counts to shadow tables appropriately.
*/
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -393,7 +393,7 @@ static mfn_t hap_make_monitor_table(stru
l4_pgentry_t *l4e;
mfn_t m4mfn;
- ASSERT(pagetable_get_pfn(v->arch.monitor_table) == 0);
+ ASSERT(pagetable_get_pfn(v->arch.hvm.monitor_table) == 0);
if ( (pg = hap_alloc(d)) == NULL )
goto oom;
@@ -579,10 +579,10 @@ void hap_teardown(struct domain *d, bool
{
if ( paging_get_hostmode(v) && paging_mode_external(d) )
{
- mfn = pagetable_get_mfn(v->arch.monitor_table);
+ mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
if ( mfn_valid(mfn) && (mfn_x(mfn) != 0) )
hap_destroy_monitor_table(v, mfn);
- v->arch.monitor_table = pagetable_null();
+ v->arch.hvm.monitor_table = pagetable_null();
}
}
}
@@ -758,10 +758,10 @@ static void hap_update_paging_modes(stru
v->arch.paging.mode = hap_paging_get_mode(v);
- if ( pagetable_is_null(v->arch.monitor_table) )
+ if ( pagetable_is_null(v->arch.hvm.monitor_table) )
{
mfn_t mmfn = hap_make_monitor_table(v);
- v->arch.monitor_table = pagetable_from_mfn(mmfn);
+ v->arch.hvm.monitor_table = pagetable_from_mfn(mmfn);
make_cr3(v, mmfn);
hvm_update_host_cr3(v);
}
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2465,10 +2465,10 @@ static void sh_update_paging_modes(struc
&SHADOW_INTERNAL_NAME(sh_paging_mode, 2);
}
- if ( pagetable_is_null(v->arch.monitor_table) )
+ if ( pagetable_is_null(v->arch.hvm.monitor_table) )
{
mfn_t mmfn = v->arch.paging.mode->shadow.make_monitor_table(v);
- v->arch.monitor_table = pagetable_from_mfn(mmfn);
+ v->arch.hvm.monitor_table = pagetable_from_mfn(mmfn);
make_cr3(v, mmfn);
hvm_update_host_cr3(v);
}
@@ -2502,10 +2502,10 @@ static void sh_update_paging_modes(struc
return;
}
- old_mfn = pagetable_get_mfn(v->arch.monitor_table);
- v->arch.monitor_table = pagetable_null();
+ old_mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
+ v->arch.hvm.monitor_table = pagetable_null();
new_mfn = v->arch.paging.mode->shadow.make_monitor_table(v);
- v->arch.monitor_table = pagetable_from_mfn(new_mfn);
+ v->arch.hvm.monitor_table = pagetable_from_mfn(new_mfn);
SHADOW_PRINTK("new monitor table %"PRI_mfn "\n",
mfn_x(new_mfn));
@@ -2724,11 +2724,11 @@ void shadow_teardown(struct domain *d, b
#ifdef CONFIG_HVM
if ( shadow_mode_external(d) )
{
- mfn_t mfn = pagetable_get_mfn(v->arch.monitor_table);
+ mfn_t mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
if ( mfn_valid(mfn) && (mfn_x(mfn) != 0) )
v->arch.paging.mode->shadow.destroy_monitor_table(v, mfn);
- v->arch.monitor_table = pagetable_null();
+ v->arch.hvm.monitor_table = pagetable_null();
}
#endif /* CONFIG_HVM */
}
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1521,7 +1521,7 @@ sh_make_monitor_table(struct vcpu *v)
{
struct domain *d = v->domain;
- ASSERT(pagetable_get_pfn(v->arch.monitor_table) == 0);
+ ASSERT(pagetable_get_pfn(v->arch.hvm.monitor_table) == 0);
/* Guarantee we can get the memory we need */
shadow_prealloc(d, SH_type_monitor_table, CONFIG_PAGING_LEVELS);
@@ -3699,7 +3699,7 @@ sh_update_linear_entries(struct vcpu *v)
/* Don't try to update the monitor table if it doesn't exist */
if ( !shadow_mode_external(d) ||
- pagetable_get_pfn(v->arch.monitor_table) == 0 )
+ pagetable_get_pfn(v->arch.hvm.monitor_table) == 0 )
return;
#if SHADOW_PAGING_LEVELS == 4
@@ -3717,7 +3717,7 @@ sh_update_linear_entries(struct vcpu *v)
{
l4_pgentry_t *ml4e;
- ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
+ ml4e = map_domain_page(pagetable_get_mfn(v->arch.hvm.monitor_table));
ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
__PAGE_HYPERVISOR_RW);
@@ -3752,7 +3752,7 @@ sh_update_linear_entries(struct vcpu *v)
l4_pgentry_t *ml4e;
l3_pgentry_t *ml3e;
int linear_slot = shadow_l4_table_offset(SH_LINEAR_PT_VIRT_START);
- ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
+ ml4e = map_domain_page(pagetable_get_mfn(v->arch.hvm.monitor_table));
ASSERT(l4e_get_flags(ml4e[linear_slot]) & _PAGE_PRESENT);
l3mfn = l4e_get_mfn(ml4e[linear_slot]);
@@ -4087,7 +4087,7 @@ sh_update_cr3(struct vcpu *v, int do_loc
///
if ( shadow_mode_external(d) )
{
- make_cr3(v, pagetable_get_mfn(v->arch.monitor_table));
+ make_cr3(v, pagetable_get_mfn(v->arch.hvm.monitor_table));
}
#if SHADOW_PAGING_LEVELS == 4
else // not shadow_mode_external...
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -583,7 +583,6 @@ struct arch_vcpu
/* guest_table holds a ref to the page, and also a type-count unless
* shadow refcounts are in use */
pagetable_t shadow_table[4]; /* (MFN) shadow(s) of guest */
- pagetable_t monitor_table; /* (MFN) hypervisor PT (for HVM) */
unsigned long cr3; /* (MA) value to install in HW CR3 */
/*
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -176,6 +176,9 @@ struct hvm_vcpu {
uint16_t p2midx;
} fast_single_step;
+ /* (MFN) hypervisor page table */
+ pagetable_t monitor_table;
+
struct hvm_vcpu_asid n1asid;
u64 msr_tsc_adjust;
First of all avoid excessive conversions. copy_{from,to}_guest(), for
example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
Further
- do_physdev_op_compat() didn't use the param form for its parameter,
- {hap,shadow}_track_dirty_vram() wrongly used the param form,
- compat processor Px logic failed to check compatibility of native and
compat structures not further converted.
As this eliminates all users of guest_handle_from_param() and as there's
no real need to allow for conversions in both directions, drop the
macros as well.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/arch/x86/compat.c
+++ b/xen/arch/x86/compat.c
@@ -15,7 +15,7 @@ typedef long ret_t;
#endif
/* Legacy hypercall (as of 0x00030202). */
-ret_t do_physdev_op_compat(XEN_GUEST_HANDLE(physdev_op_t) uop)
+ret_t do_physdev_op_compat(XEN_GUEST_HANDLE_PARAM(physdev_op_t) uop)
{
typeof(do_physdev_op) *fn =
(void *)pv_hypercall_table[__HYPERVISOR_physdev_op].native;
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -678,7 +678,7 @@ static long microcode_update_helper(void
return ret;
}
-int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
+int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
{
int ret;
struct ucode_buf *buffer;
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4441,20 +4441,16 @@ static int _handle_iomem_range(unsigned
{
if ( s > ctxt->s && !(s >> (paddr_bits - PAGE_SHIFT)) )
{
- e820entry_t ent;
- XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
- XEN_GUEST_HANDLE(e820entry_t) buffer;
-
if ( !guest_handle_is_null(ctxt->map.buffer) )
{
+ e820entry_t ent;
+
if ( ctxt->n + 1 >= ctxt->map.nr_entries )
return -EINVAL;
ent.addr = (uint64_t)ctxt->s << PAGE_SHIFT;
ent.size = (uint64_t)(s - ctxt->s) << PAGE_SHIFT;
ent.type = E820_RESERVED;
- buffer_param = guest_handle_cast(ctxt->map.buffer, e820entry_t);
- buffer = guest_handle_from_param(buffer_param, e820entry_t);
- if ( __copy_to_guest_offset(buffer, ctxt->n, &ent, 1) )
+ if ( __copy_to_guest_offset(ctxt->map.buffer, ctxt->n, &ent, 1) )
return -EFAULT;
}
ctxt->n++;
@@ -4715,8 +4711,7 @@ long arch_memory_op(unsigned long cmd, X
case XENMEM_machine_memory_map:
{
struct memory_map_context ctxt;
- XEN_GUEST_HANDLE(e820entry_t) buffer;
- XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
+ XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer;
unsigned int i;
bool store;
@@ -4732,8 +4727,7 @@ long arch_memory_op(unsigned long cmd, X
if ( store && ctxt.map.nr_entries < e820.nr_map + 1 )
return -EINVAL;
- buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t);
- buffer = guest_handle_from_param(buffer_param, e820entry_t);
+ buffer = guest_handle_cast(ctxt.map.buffer, e820entry_t);
if ( store && !guest_handle_okay(buffer, ctxt.map.nr_entries) )
return -EFAULT;
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -59,7 +59,7 @@
int hap_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
unsigned long nr,
- XEN_GUEST_HANDLE_PARAM(void) guest_dirty_bitmap)
+ XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
{
long rc = 0;
struct sh_dirty_vram *dirty_vram;
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3171,7 +3171,7 @@ static void sh_clean_dirty_bitmap(struct
int shadow_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
unsigned long nr,
- XEN_GUEST_HANDLE_PARAM(void) guest_dirty_bitmap)
+ XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
{
int rc = 0;
unsigned long end_pfn = begin_pfn + nr;
--- a/xen/arch/x86/oprofile/backtrace.c
+++ b/xen/arch/x86/oprofile/backtrace.c
@@ -74,11 +74,8 @@ dump_guest_backtrace(struct vcpu *vcpu,
}
else
{
- XEN_GUEST_HANDLE(const_frame_head_t) guest_head;
- XEN_GUEST_HANDLE_PARAM(const_frame_head_t) guest_head_param =
+ XEN_GUEST_HANDLE_PARAM(const_frame_head_t) guest_head =
const_guest_handle_from_ptr(head, frame_head_t);
- guest_head = guest_handle_from_param(guest_head_param,
- const_frame_head_t);
/* Also check accessibility of one struct frame_head beyond */
if (!guest_handle_okay(guest_head, 2))
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -285,9 +285,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
guest_from_compat_handle(data, op->u.microcode.data);
- ret = microcode_update(
- guest_handle_to_param(data, const_void),
- op->u.microcode.length);
+ ret = microcode_update(data, op->u.microcode.length);
}
break;
@@ -531,9 +529,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
XEN_GUEST_HANDLE(uint32) pdc;
guest_from_compat_handle(pdc, op->u.set_pminfo.u.pdc);
- ret = acpi_set_pdc_bits(
- op->u.set_pminfo.id,
- guest_handle_to_param(pdc, uint32));
+ ret = acpi_set_pdc_bits(op->u.set_pminfo.id, pdc);
}
break;
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -15,6 +15,7 @@ EMIT_FILE;
#define COMPAT
#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
+#define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
typedef int ret_t;
#include "../compat.c"
--- a/xen/arch/x86/x86_64/cpu_idle.c
+++ b/xen/arch/x86/x86_64/cpu_idle.c
@@ -52,13 +52,9 @@ static int copy_from_compat_state(xen_pr
compat_processor_cx_t *state)
{
#define XLAT_processor_cx_HNDL_dp(_d_, _s_) do { \
- XEN_GUEST_HANDLE(compat_processor_csd_t) dps; \
- XEN_GUEST_HANDLE_PARAM(xen_processor_csd_t) dps_param; \
if ( unlikely(!compat_handle_okay((_s_)->dp, (_s_)->dpcnt)) ) \
- return -EFAULT; \
- guest_from_compat_handle(dps, (_s_)->dp); \
- dps_param = guest_handle_cast(dps, xen_processor_csd_t); \
- (_d_)->dp = guest_handle_from_param(dps_param, xen_processor_csd_t); \
+ return -EFAULT; \
+ guest_from_compat_handle((_d_)->dp, (_s_)->dp); \
} while (0)
XLAT_processor_cx(xen_state, state);
#undef XLAT_processor_cx_HNDL_dp
--- a/xen/arch/x86/x86_64/cpufreq.c
+++ b/xen/arch/x86/x86_64/cpufreq.c
@@ -26,6 +26,8 @@
#include <xen/pmstat.h>
#include <compat/platform.h>
+CHECK_processor_px;
+
DEFINE_XEN_GUEST_HANDLE(compat_processor_px_t);
int
@@ -42,13 +44,9 @@ compat_set_px_pminfo(uint32_t cpu, struc
return -EFAULT;
#define XLAT_processor_performance_HNDL_states(_d_, _s_) do { \
- XEN_GUEST_HANDLE(compat_processor_px_t) states; \
- XEN_GUEST_HANDLE_PARAM(xen_processor_px_t) states_t; \
if ( unlikely(!compat_handle_okay((_s_)->states, (_s_)->state_count)) ) \
return -EFAULT; \
- guest_from_compat_handle(states, (_s_)->states); \
- states_t = guest_handle_cast(states, xen_processor_px_t); \
- (_d_)->states = guest_handle_from_param(states_t, xen_processor_px_t); \
+ guest_from_compat_handle((_d_)->states, (_s_)->states); \
} while (0)
XLAT_processor_performance(xen_perf, perf);
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -492,7 +492,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op
return ret;
}
-int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
+int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32) pdc)
{
u32 bits[3];
int ret;
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -40,7 +40,7 @@ int access_guest_memory_by_ipa(struct do
(XEN_GUEST_HANDLE_PARAM(type)) { _x }; \
})
-/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
+/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
#define guest_handle_to_param(hnd, type) ({ \
typeof((hnd).p) _x = (hnd).p; \
XEN_GUEST_HANDLE_PARAM(type) _y = { _x }; \
@@ -51,18 +51,6 @@ int access_guest_memory_by_ipa(struct do
_y; \
})
-
-/* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
-#define guest_handle_from_param(hnd, type) ({ \
- typeof((hnd).p) _x = (hnd).p; \
- XEN_GUEST_HANDLE(type) _y = { _x }; \
- /* type checking: make sure that the pointers inside \
- * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of \
- * the same type, then return hnd */ \
- (void)(&_x == &_y.p); \
- _y; \
-})
-
#define guest_handle_for_field(hnd, type, fld) \
((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -52,21 +52,11 @@
(XEN_GUEST_HANDLE_PARAM(type)) { _x }; \
})
-/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
+/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
#define guest_handle_to_param(hnd, type) ({ \
/* type checking: make sure that the pointers inside \
* XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of \
* the same type, then return hnd */ \
- (void)((typeof(&(hnd).p)) 0 == \
- (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
- (hnd); \
-})
-
-/* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
-#define guest_handle_from_param(hnd, type) ({ \
- /* type checking: make sure that the pointers inside \
- * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of \
- * the same type, then return hnd */ \
(void)((typeof(&(hnd).p)) 0 == \
(typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
(hnd); \
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -41,7 +41,7 @@ void hap_vcpu_init(struct vcpu *v);
int hap_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
unsigned long nr,
- XEN_GUEST_HANDLE_PARAM(void) dirty_bitmap);
+ XEN_GUEST_HANDLE(void) dirty_bitmap);
extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
int hap_set_allocation(struct domain *d, unsigned int pages, bool *preempted);
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -20,7 +20,7 @@ struct cpu_signature {
DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
void microcode_set_module(unsigned int idx);
-int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
+int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len);
int early_microcode_init(void);
int microcode_update_one(bool start_update);
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -64,7 +64,7 @@ int shadow_enable(struct domain *d, u32
int shadow_track_dirty_vram(struct domain *d,
unsigned long first_pfn,
unsigned long nr,
- XEN_GUEST_HANDLE_PARAM(void) dirty_bitmap);
+ XEN_GUEST_HANDLE(void) dirty_bitmap);
/* Handler for shadow control ops: operations from user-space to enable
* and disable ephemeral shadow modes (test mode and log-dirty mode) and
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -184,8 +184,8 @@ static inline unsigned int acpi_get_csub
static inline void acpi_set_csubstate_limit(unsigned int new_limit) { return; }
#endif
-#ifdef XEN_GUEST_HANDLE_PARAM
-int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32));
+#ifdef XEN_GUEST_HANDLE
+int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32));
#endif
int arch_acpi_set_pdc_bits(u32 acpi_id, u32 *, u32 mask);
Hi Jan,
On 21/04/2020 10:13, Jan Beulich wrote:
> First of all avoid excessive conversions. copy_{from,to}_guest(), for
> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>
> Further
> - do_physdev_op_compat() didn't use the param form for its parameter,
> - {hap,shadow}_track_dirty_vram() wrongly used the param form,
> - compat processor Px logic failed to check compatibility of native and
> compat structures not further converted.
>
> As this eliminates all users of guest_handle_from_param() and as there's
> no real need to allow for conversions in both directions, drop the
> macros as well.
I was kind of expecting both guest_handle_from_param() and
guest_handle_to_param() to be dropped together. May I ask why you still
need guest_handle_to_param()?
[...]
> /* Handler for shadow control ops: operations from user-space to enable
> * and disable ephemeral shadow modes (test mode and log-dirty mode) and
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -184,8 +184,8 @@ static inline unsigned int acpi_get_csub
> static inline void acpi_set_csubstate_limit(unsigned int new_limit) { return; }
> #endif
>
> -#ifdef XEN_GUEST_HANDLE_PARAM
> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32));
> +#ifdef XEN_GUEST_HANDLE
> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32));
> #endif
Do we really need to keep the #ifdef here?
> int arch_acpi_set_pdc_bits(u32 acpi_id, u32 *, u32 mask);
>
>
Cheers,
--
Julien Grall
On 22.04.2020 10:17, Julien Grall wrote:
> On 21/04/2020 10:13, Jan Beulich wrote:
>> First of all avoid excessive conversions. copy_{from,to}_guest(), for
>> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>>
>> Further
>> - do_physdev_op_compat() didn't use the param form for its parameter,
>> - {hap,shadow}_track_dirty_vram() wrongly used the param form,
>> - compat processor Px logic failed to check compatibility of native and
>> compat structures not further converted.
>>
>> As this eliminates all users of guest_handle_from_param() and as there's
>> no real need to allow for conversions in both directions, drop the
>> macros as well.
>
> I was kind of expecting both guest_handle_from_param() and
> guest_handle_to_param() to be dropped together. May I ask why
> you still need guest_handle_to_param()?
There are three (iirc) uses left which I don't really see how
to sensibly replace. Take a look at the different callers of
x86's vcpumask_to_pcpumask(), for example.
>> --- a/xen/include/xen/acpi.h
>> +++ b/xen/include/xen/acpi.h
>> @@ -184,8 +184,8 @@ static inline unsigned int acpi_get_csub
>> static inline void acpi_set_csubstate_limit(unsigned int new_limit) { return; }
>> #endif
>> -#ifdef XEN_GUEST_HANDLE_PARAM
>> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32));
>> +#ifdef XEN_GUEST_HANDLE
>> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32));
>> #endif
>
> Do we really need to keep the #ifdef here?
I think so, yes, or else the original one wouldn't have been
needed either. (Consider the header getting included without
any of the public headers having got included first.) Dropping
(if it was possible) this would be an orthogonal change imo.
Jan
Hi,
On 22/04/2020 10:32, Jan Beulich wrote:
> On 22.04.2020 10:17, Julien Grall wrote:
>> On 21/04/2020 10:13, Jan Beulich wrote:
>>> First of all avoid excessive conversions. copy_{from,to}_guest(), for
>>> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>>>
>>> Further
>>> - do_physdev_op_compat() didn't use the param form for its parameter,
>>> - {hap,shadow}_track_dirty_vram() wrongly used the param form,
>>> - compat processor Px logic failed to check compatibility of native and
>>> compat structures not further converted.
>>>
>>> As this eliminates all users of guest_handle_from_param() and as there's
>>> no real need to allow for conversions in both directions, drop the
>>> macros as well.
>>
>> I was kind of expecting both guest_handle_from_param() and
>> guest_handle_to_param() to be dropped together. May I ask why
>> you still need guest_handle_to_param()?
>
> There are three (iirc) uses left which I don't really see how
> to sensibly replace. Take a look at the different callers of
> x86's vcpumask_to_pcpumask(), for example.
Oh, const_guest_handle_from_ptr() is returning a GUEST_HANDLE_PARAM.
This is a bit odd but fair enough.
>
>>> --- a/xen/include/xen/acpi.h
>>> +++ b/xen/include/xen/acpi.h
>>> @@ -184,8 +184,8 @@ static inline unsigned int acpi_get_csub
>>> static inline void acpi_set_csubstate_limit(unsigned int new_limit) { return; }
>>> #endif
>>> -#ifdef XEN_GUEST_HANDLE_PARAM
>>> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32));
>>> +#ifdef XEN_GUEST_HANDLE
>>> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32));
>>> #endif
>>
>> Do we really need to keep the #ifdef here?
>
> I think so, yes, or else the original one wouldn't have been
> needed either. (Consider the header getting included without
> any of the public headers having got included first.) Dropping
> (if it was possible) this would be an orthogonal change imo.
Fair point.
Cheers,
--
Julien Grall
On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
> First of all avoid excessive conversions. copy_{from,to}_guest(), for
> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>
> Further
> - do_physdev_op_compat() didn't use the param form for its parameter,
> - {hap,shadow}_track_dirty_vram() wrongly used the param form,
> - compat processor Px logic failed to check compatibility of native and
> compat structures not further converted.
>
> As this eliminates all users of guest_handle_from_param() and as there's
> no real need to allow for conversions in both directions, drop the
> macros as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -492,7 +492,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op
> return ret;
> }
>
> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32) pdc)
Nit: switch to uint32_t while there?
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
On 22.04.2020 10:26, Roger Pau Monné wrote:
> On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
>> First of all avoid excessive conversions. copy_{from,to}_guest(), for
>> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>>
>> Further
>> - do_physdev_op_compat() didn't use the param form for its parameter,
>> - {hap,shadow}_track_dirty_vram() wrongly used the param form,
>> - compat processor Px logic failed to check compatibility of native and
>> compat structures not further converted.
>>
>> As this eliminates all users of guest_handle_from_param() and as there's
>> no real need to allow for conversions in both directions, drop the
>> macros as well.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>[...]
>> --- a/xen/drivers/acpi/pmstat.c
>> +++ b/xen/drivers/acpi/pmstat.c
>> @@ -492,7 +492,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op
>> return ret;
>> }
>>
>> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
>> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32) pdc)
>
> Nit: switch to uint32_t while there?
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Unless I hear objections, I'm intending to commit this then in a
day or two with the suggested change made and the R-b given. Of
course a formally required ack for the Arm side dropping of
guest_handle_from_param() would still be nice ...
Jan
Hi Jan,
On 05/05/2020 07:26, Jan Beulich wrote:
> On 22.04.2020 10:26, Roger Pau Monné wrote:
>> On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
>>> First of all avoid excessive conversions. copy_{from,to}_guest(), for
>>> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>>>
>>> Further
>>> - do_physdev_op_compat() didn't use the param form for its parameter,
>>> - {hap,shadow}_track_dirty_vram() wrongly used the param form,
>>> - compat processor Px logic failed to check compatibility of native and
>>> compat structures not further converted.
>>>
>>> As this eliminates all users of guest_handle_from_param() and as there's
>>> no real need to allow for conversions in both directions, drop the
>>> macros as well.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> [...]
>>> --- a/xen/drivers/acpi/pmstat.c
>>> +++ b/xen/drivers/acpi/pmstat.c
>>> @@ -492,7 +492,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op
>>> return ret;
>>> }
>>>
>>> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
>>> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32) pdc)
>>
>> Nit: switch to uint32_t while there?
>>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Unless I hear objections, I'm intending to commit this then in a
> day or two with the suggested change made and the R-b given. Of
> course a formally required ack for the Arm side dropping of
> guest_handle_from_param() would still be nice ...
I missed the small change on Arm sorry:
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
On 22.04.2020 10:26, Roger Pau Monné wrote: > On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote: >> --- a/xen/drivers/acpi/pmstat.c >> +++ b/xen/drivers/acpi/pmstat.c >> @@ -492,7 +492,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op >> return ret; >> } >> >> -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc) >> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32) pdc) > > Nit: switch to uint32_t while there? Ah, yes. > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
> First of all avoid excessive conversions. copy_{from,to}_guest(), for
> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
I'm not sure I understand the difference between those two, as they
are both placeholders for linear guest addresses?
AFAICT XEN_GUEST_HANDLE should be used for guest pointers inside of an
hypercall struct, while XEN_GUEST_HANDLE_PARAM is for guest pointers
as hypercall arguments. But those are both just guest pointers,
whether they are a parameter to the hypercall or a field in a
struct, and hence could use the same type?
I assume there's some reason for not doing so, and I see the comment
about other arches, but again a linear guest address is just that in
all arches, regardless of it's placement.
Sorry, this is likely tangential to your patch.
Thanks, Roger.
Hi,
On 21/04/2020 18:30, Roger Pau Monné wrote:
> On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
>> First of all avoid excessive conversions. copy_{from,to}_guest(), for
>> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>
> I'm not sure I understand the difference between those two, as they
> are both placeholders for linear guest addresses?
>
> AFAICT XEN_GUEST_HANDLE should be used for guest pointers inside of an
> hypercall struct, while XEN_GUEST_HANDLE_PARAM is for guest pointers
> as hypercall arguments. But those are both just guest pointers,
> whether they are a parameter to the hypercall or a field in a
> struct, and hence could use the same type?
>
> I assume there's some reason for not doing so, and I see the comment
> about other arches, but again a linear guest address is just that in
> all arches, regardless of it's placement.
On Arm:
* XEN_GUEST_HANDLE() will always be 64-bit on both 32-bit and 64-bit
hypervisor.
* XEN_GUEST_HANDLE_PARAM() will be 32-bit for 32-bit hypervisor. For
64-bit hypervisor, it will be 64-bit.
Per the ABI, each argument only fit a register. So you could not use
XEN_GUEST_HANDLE() as now an argument will be held in 2 registers on 32-bit.
We also want the structure layout to be the same for 32-bit and 64-bit.
So using XEN_GUEST_HANDLE_PARAM() everywhere is not the solution as the
virtual address is not the same.
We could possibly convert internally XEN_GUEST_HANDLE_PARAM() to
XEN_GUEST_HANDLE(), but I am not sure how this would be beneficial. We
would have to use 2 registers for arm 32-bit everytime.
Cheers,
--
Julien Grall
On Tue, Apr 21, 2020 at 07:44:55PM +0100, Julien Grall wrote:
> Hi,
>
> On 21/04/2020 18:30, Roger Pau Monné wrote:
> > On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
> > > First of all avoid excessive conversions. copy_{from,to}_guest(), for
> > > example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
> >
> > I'm not sure I understand the difference between those two, as they
> > are both placeholders for linear guest addresses?
> >
> > AFAICT XEN_GUEST_HANDLE should be used for guest pointers inside of an
> > hypercall struct, while XEN_GUEST_HANDLE_PARAM is for guest pointers
> > as hypercall arguments. But those are both just guest pointers,
> > whether they are a parameter to the hypercall or a field in a
> > struct, and hence could use the same type?
> >
> > I assume there's some reason for not doing so, and I see the comment
> > about other arches, but again a linear guest address is just that in
> > all arches, regardless of it's placement.
>
> On Arm:
> * XEN_GUEST_HANDLE() will always be 64-bit on both 32-bit and 64-bit
> hypervisor.
> * XEN_GUEST_HANDLE_PARAM() will be 32-bit for 32-bit hypervisor. For 64-bit
> hypervisor, it will be 64-bit.
>
> Per the ABI, each argument only fit a register. So you could not use
> XEN_GUEST_HANDLE() as now an argument will be held in 2 registers on 32-bit.
>
> We also want the structure layout to be the same for 32-bit and 64-bit. So
> using XEN_GUEST_HANDLE_PARAM() everywhere is not the solution as the virtual
> address is not the same.
Right, you hide the 'padding' inside XEN_GUEST_HANDLE by making it
have a fixed size on all bitnesses, I can see how that's not
desirable for hypercall params though.
Iff we ever switch to an ABI that uses physical addresses instead of
linear ones we would have to switch everything to be a 64bit integer,
or else 32bit PAE won't work correctly. Or come up with a completely
different ABI (ie: use a pre-allocated set of buffer pages, IIRC as
suggested by Juergen).
>
> We could possibly convert internally XEN_GUEST_HANDLE_PARAM() to
> XEN_GUEST_HANDLE(), but I am not sure how this would be beneficial. We would
> have to use 2 registers for arm 32-bit everytime.
Hm, we could maybe expand hypercall parameters to 64bit using some
kind of translation layer between the entry point and the actual
handler, but anyway, I get now there's a need to keep this difference.
Thanks, Roger.
Hi,
On 22/04/2020 08:56, Roger Pau Monné wrote:
> On Tue, Apr 21, 2020 at 07:44:55PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 21/04/2020 18:30, Roger Pau Monné wrote:
>>> On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
>>>> First of all avoid excessive conversions. copy_{from,to}_guest(), for
>>>> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
>>>
>>> I'm not sure I understand the difference between those two, as they
>>> are both placeholders for linear guest addresses?
>>>
>>> AFAICT XEN_GUEST_HANDLE should be used for guest pointers inside of an
>>> hypercall struct, while XEN_GUEST_HANDLE_PARAM is for guest pointers
>>> as hypercall arguments. But those are both just guest pointers,
>>> whether they are a parameter to the hypercall or a field in a
>>> struct, and hence could use the same type?
>>>
>>> I assume there's some reason for not doing so, and I see the comment
>>> about other arches, but again a linear guest address is just that in
>>> all arches, regardless of it's placement.
>>
>> On Arm:
>> * XEN_GUEST_HANDLE() will always be 64-bit on both 32-bit and 64-bit
>> hypervisor.
>> * XEN_GUEST_HANDLE_PARAM() will be 32-bit for 32-bit hypervisor. For 64-bit
>> hypervisor, it will be 64-bit.
>>
>> Per the ABI, each argument only fit a register. So you could not use
>> XEN_GUEST_HANDLE() as now an argument will be held in 2 registers on 32-bit.
>>
>> We also want the structure layout to be the same for 32-bit and 64-bit. So
>> using XEN_GUEST_HANDLE_PARAM() everywhere is not the solution as the virtual
>> address is not the same.
>
> Right, you hide the 'padding' inside XEN_GUEST_HANDLE by making it
> have a fixed size on all bitnesses, I can see how that's not
> desirable for hypercall params though.
>
> Iff we ever switch to an ABI that uses physical addresses instead of
> linear ones we would have to switch everything to be a 64bit integer,
> or else 32bit PAE won't work correctly. Or come up with a completely
> different ABI (ie: use a pre-allocated set of buffer pages, IIRC as
> suggested by Juergen).
I would go further here and request the arguments to always be the same
size regardless the bitness.
>
>>
>> We could possibly convert internally XEN_GUEST_HANDLE_PARAM() to
>> XEN_GUEST_HANDLE(), but I am not sure how this would be beneficial. We would
>> have to use 2 registers for arm 32-bit everytime.
>
> Hm, we could maybe expand hypercall parameters to 64bit using some
> kind of translation layer between the entry point and the actual
> handler, but anyway, I get now there's a need to keep this difference.
This can be done today using guest_handle_from_param manually.
Cheers,
--
Julien Grall
© 2016 - 2024 Red Hat, Inc.