Re: [Xen-devel] [PATCH v12 03/11] x86/hvm: Introduce hvm_save_cpu_ctxt_one func
On Lu, 2018-07-16 at 15:29 +, Paul Durrant wrote:
> >
> > -Original Message-
> > From: Alexandru Isaila [mailto:aisa...@bitdefender.com]
> > Sent: 16 July 2018 15:55
> > To: xen-de...@lists.xen.org
> > Cc: Ian Jackson ; Wei Liu > com>;
> > jbeul...@suse.com; Andrew Cooper ; Paul
> > Durrant ; Alexandru Isaila
> >
> > Subject: [PATCH v12 03/11] x86/hvm: Introduce hvm_save_cpu_ctxt_one
> > func
> >
> > This is used to save data from a single instance.
> >
> > Signed-off-by: Alexandru Isaila
> >
> > ---
> > Changes since V11:
> > - hvm_save_cpu_ctxt() now returns err from
> > hvm_save_cpu_ctxt_one().
> > ---
> > xen/arch/x86/hvm/hvm.c | 216 ++---
> > -
> > ---
> > 1 file changed, 113 insertions(+), 103 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index dd88751..e20a25c 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -787,119 +787,129 @@ static int hvm_load_tsc_adjust(struct
> > domain *d,
> > hvm_domain_context_t *h)
> > HVM_REGISTER_SAVE_RESTORE(TSC_ADJUST, hvm_save_tsc_adjust,
> > hvm_load_tsc_adjust, 1, HVMSR_PER_VCPU);
> >
> > +static int hvm_save_cpu_ctxt_one(struct vcpu *v,
> > hvm_domain_context_t
> > *h)
> > +{
> > +struct segment_register seg;
> > +struct hvm_hw_cpu ctxt;
> > +
> > +memset(&ctxt, 0, sizeof(ctxt));
> Why not use an = {} initializer instead of the memset here like
> elsewhere?
>
> Paul
I wanted to make less change as possible and I only added a initializer
where there was none.
Alex
> >
> > +
> > +/* Architecture-specific vmcs/vmcb bits */
> > +hvm_funcs.save_cpu_ctxt(v, &ctxt);
> > +
> > +ctxt.tsc = hvm_get_guest_tsc_fixed(v, v->domain-
> > >
> > > arch.hvm_domain.sync_tsc);
> > +
> > +ctxt.msr_tsc_aux = hvm_msr_tsc_aux(v);
> > +
> > +hvm_get_segment_register(v, x86_seg_idtr, &seg);
> > +ctxt.idtr_limit = seg.limit;
> > +ctxt.idtr_base = seg.base;
> > +
> > +hvm_get_segment_register(v, x86_seg_gdtr, &seg);
> > +ctxt.gdtr_limit = seg.limit;
> > +ctxt.gdtr_base = seg.base;
> > +
> > +hvm_get_segment_register(v, x86_seg_cs, &seg);
> > +ctxt.cs_sel = seg.sel;
> > +ctxt.cs_limit = seg.limit;
> > +ctxt.cs_base = seg.base;
> > +ctxt.cs_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_ds, &seg);
> > +ctxt.ds_sel = seg.sel;
> > +ctxt.ds_limit = seg.limit;
> > +ctxt.ds_base = seg.base;
> > +ctxt.ds_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_es, &seg);
> > +ctxt.es_sel = seg.sel;
> > +ctxt.es_limit = seg.limit;
> > +ctxt.es_base = seg.base;
> > +ctxt.es_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_ss, &seg);
> > +ctxt.ss_sel = seg.sel;
> > +ctxt.ss_limit = seg.limit;
> > +ctxt.ss_base = seg.base;
> > +ctxt.ss_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_fs, &seg);
> > +ctxt.fs_sel = seg.sel;
> > +ctxt.fs_limit = seg.limit;
> > +ctxt.fs_base = seg.base;
> > +ctxt.fs_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_gs, &seg);
> > +ctxt.gs_sel = seg.sel;
> > +ctxt.gs_limit = seg.limit;
> > +ctxt.gs_base = seg.base;
> > +ctxt.gs_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_tr, &seg);
> > +ctxt.tr_sel = seg.sel;
> > +ctxt.tr_limit = seg.limit;
> > +ctxt.tr_base = seg.base;
> > +ctxt.tr_arbytes = seg.attr;
> > +
> > +hvm_get_segment_register(v, x86_seg_ldtr, &seg);
> > +ctxt.ldtr_sel = seg.sel;
> > +ctxt.ldtr_limit = seg.limit;
> > +ctxt.ldtr_base = seg.base;
> > +ctxt.ldtr_arbytes = seg.attr;
> > +
> > +if ( v->fpu_initialised )
> > +{
> > +memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt,
> > sizeof(ctxt.fpu_regs));
> > +ctxt.flags = XEN_X86_FPU_INITIALISED;
> > +}
> > +
> > +ctxt.rax = v->arch.user_regs.rax;
> > +ctxt.rbx = v->arch.user_regs.rbx;
> > +ctxt.rcx = v->arch.user_regs.rcx;
> > +ctxt.rdx = v->arch.user_regs.rdx;
> > +ctxt.rbp = v->arch.user_regs.rbp;
> > +ctxt.rsi = v->arch.user_regs.rsi;
> > +ctxt.rdi = v->arch.user_regs.rdi;
> > +ctxt.rsp = v->arch.user_regs.rsp;
> > +ctxt.rip = v->arch.user_regs.rip;
> > +ctxt.rflags = v->arch.user_regs.rflags;
> > +ctxt.r8 = v->arch.user_regs.r8;
> > +ctxt.r9 = v->arch.user_regs.r9;
> > +ctxt.r10 = v->arch.user_regs.r10;
> > +ctxt.r11 = v->arch.user_regs.r11;
> > +ctxt.r12 = v->arch.user_regs.r12;
> > +ctxt.r13 = v->arch.user_regs.r13;
> > +ctxt.r14 = v->arch.user_regs.r14;
> > +ctxt.r15 = v->arch.user_regs.r15;
> > +ctxt.dr0 = v->arch.debugreg[0];
> > +ctxt.dr1 = v->arch.debugreg[1];
> > +ctxt.dr2 = v->arch.debugreg[2];
> > +ctxt.dr3 = v->arch.debugreg[3];
> > +ctxt.dr6 = v->arch.debugreg[6];
> > +ctxt.dr7 = v->arch.debugreg
[Xen-devel] PING: [PATCH v3] x86/mm: Add mem access rights to NPT
Any thoughts on this patch are appreciated.
Thanks,
Alex
On Lu, 2018-07-02 at 15:42 +0300, Alexandru Isaila wrote:
> From: Isaila Alexandru
>
> This patch adds access rights for the NPT pages. The access rights
> are
> saved in a radix tree with the root saved in p2m_domain. The rights
> are manipulated through p2m_set_access()
> and p2m_get_access() functions.
> The patch follows the ept logic.
>
> Note: It was tested with xen-access write
>
> Signed-off-by: Alexandru Isaila
>
> ---
> Changes since V2:
> - Delete blak line
> - Add return if p2m_access_rwx = a
> - Delete the comment from p2m_pt_get_entry()
> - Moved radix_tree_init() to arch_monitor_init_domain().
> ---
> xen/arch/x86/mm/mem_access.c | 3 ++
> xen/arch/x86/mm/p2m-pt.c | 109
> ++-
> xen/arch/x86/mm/p2m.c| 6 +++
> xen/arch/x86/monitor.c | 13 +
> xen/include/asm-x86/mem_access.h | 2 +-
> xen/include/asm-x86/p2m.h| 6 +++
> 6 files changed, 124 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c
> b/xen/arch/x86/mm/mem_access.c
> index c0cd017..d78c82c 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned
> long gla,
> {
> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> req->u.mem_access.gla = gla;
> +}
>
> +if ( npfec.gla_valid || cpu_has_svm )
> +{
> if ( npfec.kind == npfec_kind_with_gla )
> req->u.mem_access.flags |=
> MEM_ACCESS_FAULT_WITH_GLA;
> else if ( npfec.kind == npfec_kind_in_gpt )
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index b8c5d2e..4330d1f 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -68,7 +68,8 @@
> static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
> p2m_type_t t,
> mfn_t mfn,
> - unsigned int level)
> + unsigned int level,
> + p2m_access_t access)
> {
> unsigned long flags;
> /*
> @@ -87,23 +88,27 @@ static unsigned long p2m_type_to_flags(const
> struct p2m_domain *p2m,
> case p2m_ram_paged:
> case p2m_ram_paging_in:
> default:
> -return flags | _PAGE_NX_BIT;
> +flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
> +break;
> case p2m_grant_map_ro:
> return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
> case p2m_ioreq_server:
> flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> -return flags & ~_PAGE_RW;
> -return flags;
> +flags &= ~_PAGE_RW;
> +break;
> case p2m_ram_ro:
> case p2m_ram_logdirty:
> case p2m_ram_shared:
> -return flags | P2M_BASE_FLAGS;
> +flags |= P2M_BASE_FLAGS;
> +break;
> case p2m_ram_rw:
> -return flags | P2M_BASE_FLAGS | _PAGE_RW;
> +flags |= P2M_BASE_FLAGS | _PAGE_RW;
> +break;
> case p2m_grant_map_rw:
> case p2m_map_foreign:
> -return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> +flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> +break;
> case p2m_mmio_direct:
> if ( !rangeset_contains_singleton(mmio_ro_ranges,
> mfn_x(mfn)) )
> flags |= _PAGE_RW;
> @@ -112,8 +117,37 @@ static unsigned long p2m_type_to_flags(const
> struct p2m_domain *p2m,
> flags |= _PAGE_PWT;
> ASSERT(!level);
> }
> -return flags | P2M_BASE_FLAGS | _PAGE_PCD;
> +flags |= P2M_BASE_FLAGS | _PAGE_PCD;
> +break;
> +}
> +switch ( access )
> +{
> +case p2m_access_r:
> +case p2m_access_w:
> +flags |= _PAGE_NX_BIT;
> +flags &= ~_PAGE_RW;
> +break;
> +case p2m_access_rw:
> +flags |= _PAGE_NX_BIT;
> +break;
> +case p2m_access_n:
> +case p2m_access_n2rwx:
> +flags |= _PAGE_NX_BIT;
> +flags &= ~_PAGE_RW;
> +break;
> +case p2m_access_rx:
> +case p2m_access_wx:
> +case p2m_access_rx2rw:
> +flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
> +break;
> +case
Re: [Xen-devel] [PATCH v12 03/11] x86/hvm: Introduce hvm_save_cpu_ctxt_one func
On Ma, 2018-07-17 at 08:03 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 17.07.18 at 14:25, wrote:
> > On Lu, 2018-07-16 at 15:29 +, Paul Durrant wrote:
> > >
> > > >
> > > > From: Alexandru Isaila [mailto:aisa...@bitdefender.com]
> > > > Sent: 16 July 2018 15:55
> > > > --- a/xen/arch/x86/hvm/hvm.c
> > > > +++ b/xen/arch/x86/hvm/hvm.c
> > > > @@ -787,119 +787,129 @@ static int hvm_load_tsc_adjust(struct
> > > > domain *d,
> > > > hvm_domain_context_t *h)
> > > > HVM_REGISTER_SAVE_RESTORE(TSC_ADJUST, hvm_save_tsc_adjust,
> > > > hvm_load_tsc_adjust, 1,
> > > > HVMSR_PER_VCPU);
> > > >
> > > > +static int hvm_save_cpu_ctxt_one(struct vcpu *v,
> > > > hvm_domain_context_t
> > > > *h)
> > > > +{
> > > > +struct segment_register seg;
> > > > +struct hvm_hw_cpu ctxt;
> > > > +
> > > > +memset(&ctxt, 0, sizeof(ctxt));
> > > Why not use an = {} initializer instead of the memset here like
> > > elsewhere?
> > I wanted to make less change as possible and I only added a
> > initializer
> > where there was none.
> Trying to limit patch impact is certainly appreciated, but please
> take a
> look at your patch to see whether this would really have made much
> of a difference.
>
I will change this in the next version but I will wait for more
comments on the rest of the patches.
Regards,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
>
> >
> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila > om> wrote:
> >
> > From: Isaila Alexandru
> >
> > This patch adds access rights for the NPT pages. The access rights
> > are
> > saved in a radix tree with the root saved in p2m_domain. The rights
> > are manipulated through p2m_set_access()
> > and p2m_get_access() functions.
> > The patch follows the ept logic.
> This description needs to be much more complete. Something like
> this:
>
> ---
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT
> p2m table, so we add a radix tree to store the rights. For
> efficiency, remove entries which match the default permissions rather
> than continuing to store them.
>
> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking
> an access, and removing / adding RW or NX flags as appropriate.
> ---
>
I will update the patch description.
> >
> >
> > Note: It was tested with xen-access write
> >
> > Signed-off-by: Alexandru Isaila
>
> >
> >
> > ---
> > Changes since V2:
> > - Delete blak line
> > - Add return if p2m_access_rwx = a
> > - Delete the comment from p2m_pt_get_entry()
> > - Moved radix_tree_init() to arch_monitor_init_domain().
> > ---
> > xen/arch/x86/mm/mem_access.c | 3 ++
> > xen/arch/x86/mm/p2m-pt.c | 109
> > ++-
> > xen/arch/x86/mm/p2m.c| 6 +++
> > xen/arch/x86/monitor.c | 13 +
> > xen/include/asm-x86/mem_access.h | 2 +-
> > xen/include/asm-x86/p2m.h| 6 +++
> > 6 files changed, 124 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_access.c
> > b/xen/arch/x86/mm/mem_access.c
> > index c0cd017..d78c82c 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa,
> > unsigned long gla,
> > {
> > req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> > req->u.mem_access.gla = gla;
> > +}
> >
> > +if ( npfec.gla_valid || cpu_has_svm )
> > +{
> I can’t immediately tell what this is about, which means it needs a
> comment.
>
> It may also be (depending on why this is here) that “cpu_has_svm”
> makes this more fragile — if this is really about having a radix
> tree, for instance, then you should probably check for a radix tree.
This is about the different npfec on SVN. The gla in never valid so the
fault flag will not be set.
>
> >
> > if ( npfec.kind == npfec_kind_with_gla )
> > req->u.mem_access.flags |=
> > MEM_ACCESS_FAULT_WITH_GLA;
> > else if ( npfec.kind == npfec_kind_in_gpt )
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> > index b8c5d2e..4330d1f 100644
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -68,7 +68,8 @@
> > static unsigned long p2m_type_to_flags(const struct p2m_domain
> > *p2m,
> > p2m_type_t t,
> > mfn_t mfn,
> > - unsigned int level)
> > + unsigned int level,
> > + p2m_access_t access)
> > {
> > unsigned long flags;
> > /*
> > @@ -87,23 +88,27 @@ static unsigned long p2m_type_to_flags(const
> > struct p2m_domain *p2m,
> > case p2m_ram_paged:
> > case p2m_ram_paging_in:
> > default:
> > -return flags | _PAGE_NX_BIT;
> > +flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
> > +break;
> > case p2m_grant_map_ro:
> > return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
> > case p2m_ioreq_server:
> > flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> > if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> > -return flags & ~_PAGE_RW;
> > -return flags;
> > +flags &= ~_PAGE_RW;
> > +break;
> > case p2m_ram_ro:
> > case p2m_ram_logdirty:
> > case p2m_ram_shared:
> > -return flags | P2M_BASE_FLAGS;
> > +flags |= P2M_BASE_FLAGS;
> > +break;
> > case p2m_ram_rw:
> > -return flags | P
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 19.07.18 at 10:43, wrote:
> > On 07/19/2018 11:30 AM, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On 19.07.18 at 10:18, wrote:
> > > > On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
> > > > >
> > > > > >
> > > > > > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila > > > > > fender.c
> > > > > > +break;
> > > > > > +case p2m_access_x:
> > > > > > +flags &= ~_PAGE_RW;
> > > > > > +break;
> > > > > > +case p2m_access_rwx:
> > > > > > +default:
> > > > > > +break;
> > > > > > }
> > > > > I think you want another blank line here too.
> > > > >
> > > > > Also, this doesn’t seem to capture the ‘r’ part of the
> > > > > equation —
> > > > > shouldn’t p2m_access_n end up with a not-present p2m entry?
> > > > SVM dosen't explicitly provide a read access bit so we treat
> > > > read and
> > > > write the same way.
> > > Read and write can't possibly be treated the same. You ought to
> > > use
> > > the present bit to deny read (really: any) access, as also
> > > implied by
> > > George's response.
> > They aren't treated the same as far sending out a vm_event goes.
> > However, if we understand this correctly, there is no way to cause
> > only
> > read, or only write exits for NPT. They are bundled together under
> > _PAGE_RW.
> >
> > So svm_do_nested_pgfault() tries to sort these out:
> >
> > 1781 struct npfec npfec = {
> > 1782 .read_access = !(pfec & PFEC_insn_fetch),
> > 1783 .write_access = !!(pfec & PFEC_write_access),
> > 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
> > 1785 .present = !!(pfec & PFEC_page_present),
> > 1786 };
> > 1787
> > 1788 /* These bits are mutually exclusive */
> > 1789 if ( pfec & NPT_PFEC_with_gla )
> > 1790 npfec.kind = npfec_kind_with_gla;
> > 1791 else if ( pfec & NPT_PFEC_in_gpt )
> > 1792 npfec.kind = npfec_kind_in_gpt;
> > 1793
> > 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
> >
> > but a read access is considered to be something that's not an insn
> > fetch, and we only have a specific bit set for the write.
> >
> > Since hvm_hap_nested_page_fault() looks at npfec to decide when to
> > send
> > out a vm_event, this takes care of handling reads and writes
> > differently
> > at this level; however it's not possible to set separate single
> > "don't
> > read" or "don't write" exit-causing flags with NPT.
> All fine, but George's question was raised in the context of
> permission
> conversion from p2m to pte representation.
I've tried a test with xen access that sets XENMEM_access_n on all the
pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
the p2m_access_n case and the guest fails with triple fault. There are
a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
invalid if the flag is not present. I don't think this is the way to go
with the p2m_access_n flags.
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
> I will absolutely nack any interface where if the caller says, > "Please > remove read permission", the hypervisor says, "OK!" but then allows > read > permission anyway -- particularly in one which is allegedly designed > for > security tools. > > If it's not practical / more work than it's worth doing at the moment > to > implement p2m_access_n on NPT, then you should return an error when > it's > requested. > > The same really should be true for write-only permission as well -- > if > it's not possible to allow writes but not reads, then you should > return > an error when such permissions are requested. I will limit the supported access rights and return error for read/write only and _n. Regards, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] x86/mm: Add mem access rights to NPT
> >
> > +static void p2m_set_access(struct p2m_domain *p2m, unsigned long
> > gfn,
> > + p2m_access_t a)
> > +{
> > +int rc;
> > +
> > +if ( !p2m->mem_access_settings )
> > +return;
> No error indication?
I would say ASSERT is a better choice if the code got this far and it
could not allocate memory
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] x86/mm: Add mem access rights to NPT
On Mi, 2018-07-25 at 03:14 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 25.07.18 at 10:29, wrote:
> > > >
> > > > +static void p2m_set_access(struct p2m_domain *p2m, unsigned
> > > > long
> > > > gfn,
> > > > + p2m_access_t a)
> > > > +{
> > > > +int rc;
> > > > +
> > > > +if ( !p2m->mem_access_settings )
> > > > +return;
> > > No error indication?
> > I would say ASSERT is a better choice if the code got this far and
> > it
> > could not allocate memory
> For one ASSERT() is a no-op in release builds. And then it is
> extremely bad practices to bring down the host when an operation
> targeting just a single guest has failed. You either return an error
> indicator here (and pass it up the call tree), or if that's really
> unfeasible then you crash the affected domain (we do so in quite
> a few other situations). But you'd need to make clear (if it's not
> obvious) why passing up an error is unacceptable here.
>
By this time in the code the radix tree should be in place. If it is
not then the domain should crash because something is wrong and the mem
access feature will not function so passing the error up will have a
result of crashing the domain later after checking.
I will add a domain crash here and a comment regarding it.
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v14 08/11] x86/hvm: Add handler for save_one funcs
On Ma, 2018-07-31 at 06:34 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 25.07.18 at 14:14, wrote:
> > --- a/xen/arch/x86/hvm/save.c
> > +++ b/xen/arch/x86/hvm/save.c
> > @@ -85,16 +85,18 @@ int arch_hvm_load(struct domain *d, struct
> > hvm_save_header *hdr)
> > /* List of handlers for various HVM save and restore types */
> > static struct {
> > hvm_save_handler save;
> > +hvm_save_one_handler save_one;
> > hvm_load_handler load;
> > const char *name;
> > size_t size;
> > int kind;
> > -} hvm_sr_handlers[HVM_SAVE_CODE_MAX + 1] = { {NULL, NULL, ""},
> > };
> > +} hvm_sr_handlers[HVM_SAVE_CODE_MAX + 1] = { {NULL, NULL, NULL,
> > ""}, };
> This initializer looks flawed, and I'd appreciate if you could fix it
> as
> you have to touch it anyway: It only sets .name of array entry 0
> to a non-NULL string. Either this setting is not needed in the first
> place (as looks to be the case), or you'll want to initialize all
> array
> entries the same. Use the C99 (GNU extension in C89) for that
> purpose. Perhaps simply dropping the initializer could be prereq
> patch which could go in right away.
>
> >
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1576,9 +1576,9 @@ static int lapic_load_regs(struct domain *d,
> > hvm_domain_context_t *h)
> > return 0;
> > }
> >
> > -HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden,
> > lapic_load_hidden,
> > +HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, NULL,
> > lapic_load_hidden,
> > 1, HVMSR_PER_VCPU);
> > -HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs,
> > lapic_load_regs,
> > +HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, NULL,
> > lapic_load_regs,
> These are per-vCPU as well - why do they get NULL inserted here,
> rather than there being another (two) prereq patch(es)?
Both LAPIC save functions have for for (vcpu) so the look like a
save_one function already, no need to do anything here.
>
> >
> > --- a/xen/include/asm-x86/hvm/save.h
> > +++ b/xen/include/asm-x86/hvm/save.h
> > @@ -97,6 +97,8 @@ static inline uint16_t hvm_load_instance(struct
> > hvm_domain_context *h)
> > * restoring. Both return non-zero on error. */
> > typedef int (*hvm_save_handler) (struct domain *d,
> > hvm_domain_context_t *h);
> > +typedef int (*hvm_save_one_handler)(struct vcpu *v,
> > +hvm_domain_context_t *h);
> I don't think "one" is a valid part of the name here: PIC has
> multiple instances too (and eventually IO-APIC will), yet they're
> not to be managed this way. I think you want to use "vcpu"
> instead.
>
> >
> > @@ -114,12 +117,13 @@ void hvm_register_savevm(uint16_t typecode,
> >
> > /* Syntactic sugar around that function: specify the max number of
> > * saves, and this calculates the size of buffer needed */
> > -#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num,
> > _k) \
> > +#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _save_one, _load,
> > _num, _k) \
> > static int __init
> > __hvm_register_##_x##_save_and_restore(void)\
> > {
> > \
> > hvm_register_savevm(HVM_SAVE_CODE(_x),
> > \
> > #_x,
> > \
> > &_save,
> > \
> > +_save_one,
> > \
> While I generally appreciate the omission of the &, I'd
> prefer if you added it for consistency with the neighboring
> lines.
This was done so we can add NULL in the places that do not have
save_one functions.
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v14 10/11] x86/hvm: Remove redundant save functions
On Ma, 2018-07-31 at 07:24 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 25.07.18 at 14:14, wrote:
> > --- a/xen/arch/x86/hvm/hpet.c
> > +++ b/xen/arch/x86/hvm/hpet.c
> > @@ -516,8 +516,9 @@ static const struct hvm_mmio_ops hpet_mmio_ops
> > = {
> > };
> >
> >
> > -static int hpet_save(struct domain *d, hvm_domain_context_t *h)
> > +static int hpet_save(struct vcpu *vcpu, hvm_domain_context_t *h)
> Consistently v please.
>
Here the hpet_save() works with a local defined struct vcpu* v so I
have struct vcpu *vcpu in order to change as little code as possible.
If the v needs to be constant then I will change it in the next
version.
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v14 08/11] x86/hvm: Add handler for save_one funcs
On Ma, 2018-07-31 at 07:32 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 31.07.18 at 14:55, wrote:
> > On Ma, 2018-07-31 at 06:34 -0600, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On 25.07.18 at 14:14, wrote:
> > > > --- a/xen/arch/x86/hvm/vlapic.c
> > > > +++ b/xen/arch/x86/hvm/vlapic.c
> > > > @@ -1576,9 +1576,9 @@ static int lapic_load_regs(struct domain
> > > > *d,
> > > > hvm_domain_context_t *h)
> > > > return 0;
> > > > }
> > > >
> > > > -HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden,
> > > > lapic_load_hidden,
> > > > +HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, NULL,
> > > > lapic_load_hidden,
> > > > 1, HVMSR_PER_VCPU);
> > > > -HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs,
> > > > lapic_load_regs,
> > > > +HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, NULL,
> > > > lapic_load_regs,
> > > These are per-vCPU as well - why do they get NULL inserted here,
> > > rather than there being another (two) prereq patch(es)?
> > Both LAPIC save functions have for for (vcpu) so the look like a
> > save_one function already, no need to do anything here.
> Quite the opposite - presence of a loop over all vCPU-s clearly
> says they're not save-one functions. Otherwise you wouldn't
> have found the need to touch the functions the way you do in
> patch 10.
>
> >
> > >
> > > >
> > > > @@ -114,12 +117,13 @@ void hvm_register_savevm(uint16_t
> > > > typecode,
> > > >
> > > > /* Syntactic sugar around that function: specify the max
> > > > number of
> > > > * saves, and this calculates the size of buffer needed */
> > > > -#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num,
> > > > _k) \
> > > > +#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _save_one, _load,
> > > > _num, _k) \
> > > > static int __init
> > > > __hvm_register_##_x##_save_and_restore(void)\
> > > > {
> > > >
> > > > \
> > > > hvm_register_savevm(HVM_SAVE_CODE(_x),
> > > >
> > > > \
> > > > #_x,
> > > >
> > > > \
> > > > &_save,
> > > >
> > > > \
> > > > +_save_one,
> > > >
> > > > \
> > > While I generally appreciate the omission of the &, I'd
> > > prefer if you added it for consistency with the neighboring
> > > lines.
> > This was done so we can add NULL in the places that do not have
> > save_one functions.
> ??? (I cannot connect your response to my remark.)
>
If there is &_save_one then it will not compile if there is any call
with a NULL.
hpet.c: In function ‘__hvm_register_HPET_save_and_restore’:
/home/aisaila/work/xen/xen/include/asm/hvm/save.h:126:25: error: lvalue
required as unary ‘&’ operand
&_save_one,
\
^
hpet.c:643:1: note: in expansion of macro ‘HVM_REGISTER_SAVE_RESTORE’
HVM_REGISTER_SAVE_RESTORE(HPET, hpet_save, NULL, hpet_load, 1,
HVMSR_PER_DOM);
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v14 09/11] x86/domctl: Don't pause the whole domain if only getting vcpu state
On Ma, 2018-07-31 at 07:08 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 25.07.18 at 14:14, wrote: > > This patch is focused on moving the for loop to the caller so > > now we can save info for a single vcpu instance with the save_one > > handlers. > > > > Signed-off-by: Alexandru Isaila > First of all I'd appreciate if this patch was last in the series, > after all > infrastructure changes have been done. > If this patch will be the last one in the series then between the one that removes the save functions (and with that the for) and this one the code will not run as expected because there will be no for. This patch has to be followed by the "Remove redundant save functions" and not the other way around. Please clarify this if I did not understood correctly. Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v14 06/11] x86/hvm: Introduce hvm_save_mtrr_msr_one func
On Ma, 2018-07-31 at 06:16 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 25.07.18 at 14:14, wrote:
> > This is used to save data from a single instance.
> >
> > Signed-off-by: Alexandru Isaila
> >
> > ---
> > Changes since v11:
> > - hvm_save_mtrr_msr() now returns err from
> > hvm_save_mtrr_msr_one().
> >
> > Note: This patch is based on Roger Pau Monne's series[1]
> > ---
> > xen/arch/x86/hvm/mtrr.c | 81 +++
> > --
> > 1 file changed, 44 insertions(+), 37 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> > index 48facbb..47a5c29 100644
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -718,52 +718,59 @@ int hvm_set_mem_pinned_cacheattr(struct
> > domain *d,
> > uint64_t gfn_start,
> > return 0;
> > }
> >
> > -static int hvm_save_mtrr_msr(struct domain *d,
> > hvm_domain_context_t *h)
> > +static int hvm_save_mtrr_msr_one(struct vcpu *v,
> > hvm_domain_context_t *h)
> > {
> > -struct vcpu *v;
> > +const struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
> > +struct hvm_hw_mtrr hw_mtrr = {
> > +.msr_mtrr_def_type = mtrr_state->def_type |
> > + MASK_INSR(mtrr_state->fixed_enabled,
> > + MTRRdefType_FE) |
> > + MASK_INSR(mtrr_state->enabled,
> > MTRRdefType_E),
> > +.msr_mtrr_cap = mtrr_state->mtrr_cap,
> > +};
> > +unsigned int i;
> >
> > -/* save mtrr&pat */
> > -for_each_vcpu(d, v)
> > +if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
> > + (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
> > {
> > -const struct mtrr_state *mtrr_state = &v-
> > >arch.hvm_vcpu.mtrr;
> > -struct hvm_hw_mtrr hw_mtrr = {
> > -.msr_mtrr_def_type = mtrr_state->def_type |
> > - MASK_INSR(mtrr_state-
> > >fixed_enabled,
> > - MTRRdefType_FE) |
> > - MASK_INSR(mtrr_state->enabled,
> > MTRRdefType_E),
> > -.msr_mtrr_cap = mtrr_state->mtrr_cap,
> > -};
> > -unsigned int i;
> > +dprintk(XENLOG_G_ERR,
> > +"HVM save: %pv: too many (%lu) variable range
> > MTRRs\n",
> > +v, MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT));
> > +return -EINVAL;
> > +}
> >
> > -if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
> > - (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
> > -{
> > -dprintk(XENLOG_G_ERR,
> > -"HVM save: %pv: too many (%lu) variable range
> > MTRRs\n",
> > -v, MASK_EXTR(hw_mtrr.msr_mtrr_cap,
> > MTRRcap_VCNT));
> > -return -EINVAL;
> > -}
> > +hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
> >
> > -hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
> > +for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap,
> > MTRRcap_VCNT); i++ )
> > +{
> > +/* save physbase */
> > +hw_mtrr.msr_mtrr_var[i*2] =
> > +((uint64_t*)mtrr_state->var_ranges)[i*2];
> > +/* save physmask */
> > +hw_mtrr.msr_mtrr_var[i*2+1] =
> > +((uint64_t*)mtrr_state->var_ranges)[i*2+1];
> > +}
> As you move/re-indent code, please fix obvious style violations
> (here: missing blanks around binary operators). It also would be
> really nice if you got rid of the ugly and risky casts, and used
> the actual structure fields instead.
Couldn't we just use a
memcpy(hw_mtrr.msr_mtrr_var, mtrr_state->var_ranges,
MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT)) ?
Same as you suggested...
>
> >
> > -for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap,
> > MTRRcap_VCNT); i++ )
> > -{
> > -/* save physbase */
> > -hw_mtrr.msr_mtrr_var[i*2] =
> > -((uint64_t*)mtrr_state->var_ranges)[i*2];
> > -/* save physmask */
> > -hw_mtrr.msr_mtrr_var[i*2+1] =
> > -((uint64_t*)mtrr_state->var_ranges)[i*2+1];
> > -}
> > +for ( i = 0; i < NUM_FIXED_MSR; i++ )
> > +hw_mtrr.msr_mtrr_fixed[i] =
> > +((uint64_t*)mtrr_state->fixed_ranges)[i];
> Whereas here I think you would best simply use memcpy(), again
> to get rid of the cast.
>
... here
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v15 09/14] x86/hvm: Introduce lapic_save_regs_one func
On Ma, 2018-08-07 at 06:09 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 03.08.18 at 15:53, wrote:
> > This is used to save data from a single instance.
> >
> > Signed-off-by: Alexandru Isaila
> > ---
> > xen/arch/x86/hvm/vlapic.c | 27 +++
> > 1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 0795161..d35810e 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1460,26 +1460,37 @@ static int lapic_save_hidden(struct domain
> > *d,
> > hvm_domain_context_t *h)
> > return err;
> > }
> >
> > +static int lapic_save_regs_one(struct vcpu *v,
> > hvm_domain_context_t *h)
> > +{
> > +struct vlapic *s;
> > +
> > +if ( !has_vlapic(v->domain) )
> > +return 0;
> > +
> > +if ( hvm_funcs.sync_pir_to_irr )
> > +hvm_funcs.sync_pir_to_irr(v);
> > +
> > +s = vcpu_vlapic(v);
> > +
> > +return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs);
> > +}
> Here as well as in patch 8 there's little point in having a local
> variable s
> which is used just once. If you really think you want to retain them,
> here it can be pointer to const (other than in patch 8 afaict), and
> like
> in patch 8 it could have an initializer instead of later having a
> separate
> assignment statement.
>
> >
> > static int lapic_save_regs(struct domain *d, hvm_domain_context_t
> > *h)
> > {
> > struct vcpu *v;
> > -struct vlapic *s;
> > -int rc = 0;
> > +int err = 0;
> >
> > if ( !has_vlapic(d) )
> > return 0;
> >
> > for_each_vcpu ( d, v )
> > {
> > -if ( hvm_funcs.sync_pir_to_irr )
> > -hvm_funcs.sync_pir_to_irr(v);
> > -
> > -s = vcpu_vlapic(v);
> > -if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s-
> > >regs)) != 0 )
> > +err = lapic_save_regs_one(v, h);
> > +if ( err )
> > break;
> > }
> >
> > -return rc;
> > +return err;
> > }
> Since the whole function is meant to go away anyway, it doesn't
> matter much, but why did you see a need to replace "rc" by "err"?
> This only increases code churn (even if just slightly). IOW: No
> need to change this, but something to consider in the future.
>
Err was just to have all the functions work with the same variable name
so this was done just for consistency.
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v15 06/14] x86/hvm: Introduce hvm_save_mtrr_msr_one func
> > > > > -hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr); > > +memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges, > > NUM_FIXED_MSR); > You want to BUILD_BUG_ON() array sizes differing, and then use > sizeof() in the call to memcpy(). > In this case sizes are different: msr_mtrr_fixed[NUM_FIXED_MSR]; fixed_ranges[NUM_FIXED_RANGES]; #define NUM_FIXED_RANGES 88 #define NUM_FIXED_MSR 11 so it will most likely assert a message. Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] x86/mm: Add mem access rights to NPT
On Mi, 2018-07-25 at 04:37 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 25.07.18 at 11:25, wrote:
> > On 07/24/2018 01:02 PM, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On 24.07.18 at 13:26, wrote:
> > > > On 07/24/2018 09:55 AM, Jan Beulich wrote:
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > On 23.07.18 at 15:48, wrote:
> > > > > > +{
> > > > > > +xfree(d->arch.monitor.msr_bitmap);
> > > > > > +return -ENOMEM;
> > > > > > +}
> > > > > > +radix_tree_init(p2m->mem_access_settings);
> > > > > > +}
> > > > > What's the SVM connection here? Please don't forget that p2m-
> > > > > pt.c
> > > > > also serves the shadow case. Perhaps struct p2m_domain should
> > > > > contain a boolean indicator whether this auxiliary data
> > > > > structure is
> > > > > needed?
> > > > It's basically just "hap_enabled()" isn't it?
> > > Only if we can't make it there when EPT is active.
> > It can make it here when VMX is active and shadow is enabled, but
> > it
> > shouldn't be able to get here when EPT is active. We could add an
> > ASSERT() to that effect; it should be safe in production, as the
> > only
> > side effect should be that we do a small pointless allocation.
> So I've looked a little more closely: This is being added to
> arch_monitor_init_domain(), called from vm_event_domctl(). I can't
> see why this wouldn't be reachable with EPT enabled.
>
Hi George,
Any thoughts on this?
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v17 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state
On Mi, 2018-08-22 at 16:41 +0200, Roger Pau Monné wrote: > On Wed, Aug 22, 2018 at 05:02:43PM +0300, Alexandru Isaila wrote: > > > > This patch is focused on moving changing hvm_save_one() to save one > > typecode from one vcpu and now that the save functions get data > > from a > > single vcpu we can pause the specific vcpu instead of the domain. > With this infrastructure added allowing to save a single instance of > a > specific device I wonder if you would like to add a user to the code > in the tree. > > If you look at vcpu_hvm in tools/libxc/xc_dom_x86.c it saves the full > domain context just to get the CPU and the MTRR state of VCPU#0. Do > you think you could switch this code to use the newly introduced > machinery to save a single instance of a specific type? Sure, I will add a tool patch at the end of the series > > > > > Signed-off-by: Alexandru Isaila > > > > --- > > Changes since V15: > > - Moved pause/unpause calls into hvm_save_one() > > - Re-add the loop in hvm_save_one(). > > --- > > xen/arch/x86/domctl.c | 2 -- > > xen/arch/x86/hvm/save.c | 12 ++-- > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > > index 8fbbf3a..cb53980 100644 > > --- a/xen/arch/x86/domctl.c > > +++ b/xen/arch/x86/domctl.c > > @@ -591,12 +591,10 @@ long arch_do_domctl( > > !is_hvm_domain(d) ) > > break; > > > > -domain_pause(d); > > ret = hvm_save_one(d, domctl->u.hvmcontext_partial.type, > > domctl->u.hvmcontext_partial.instance, > > domctl->u.hvmcontext_partial.buffer, > > &domctl->u.hvmcontext_partial.bufsz); > > -domain_unpause(d); > > > > if ( !ret ) > > copyback = true; > > diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c > > index 49741e0..2d35f17 100644 > > --- a/xen/arch/x86/hvm/save.c > > +++ b/xen/arch/x86/hvm/save.c > > @@ -149,12 +149,15 @@ int hvm_save_one(struct domain *d, unsigned > > int typecode, unsigned int instance, > > instance >= d->max_vcpus ) > > return -ENOENT; > > ctxt.size = hvm_sr_handlers[typecode].size; > > -if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU ) > > -ctxt.size *= d->max_vcpus; > This chunk seems to belong to a different patch? > > The change just mentions pausing a vpcu instead of the whole domain, > but the size of the save context doesn't depend on whether the whole > domain is paused vs a single vcpu is paused. Right, git rebase did not report any error so it tricked me. I will have that removed from this patch. Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v17 12/13] x86/hvm: Remove redundant save functions
> >
> > -if ( handler(d, h) != 0 )
> > +if ( handler(d->vcpu[0], h) != 0 )
> > {
> > printk(XENLOG_G_ERR
> > - "HVM%d save: failed to save type
> > %"PRIu16"\n",
> > + "HVM d%d save: failed to save type
> > %"PRIu16"\n",
> This looks like an unrelated change.
I wanted to change this so it could be in the same order as the
per_vcpu print that was modified at Jan's request.
Now the print is something like:
"
(XEN) HVM d2v0 save: CPU
(XEN) HVM d2v1 save: CPU
(XEN) HVM d2 save: PIC
(XEN) HVM d2 save: IOAPIC
(XEN) HVM d2v0 save: LAPIC
"
in oppose to having "(XEN) HVM2 save: PIC".
If the change is not needed I can drop it.
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v17 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state
On Mi, 2018-08-22 at 18:15 +0300, Isaila Alexandru wrote: > On Mi, 2018-08-22 at 16:41 +0200, Roger Pau Monné wrote: > > > > On Wed, Aug 22, 2018 at 05:02:43PM +0300, Alexandru Isaila wrote: > > > > > > > > > This patch is focused on moving changing hvm_save_one() to save > > > one > > > typecode from one vcpu and now that the save functions get data > > > from a > > > single vcpu we can pause the specific vcpu instead of the domain. > > With this infrastructure added allowing to save a single instance > > of > > a > > specific device I wonder if you would like to add a user to the > > code > > in the tree. > > > > If you look at vcpu_hvm in tools/libxc/xc_dom_x86.c it saves the > > full > > domain context just to get the CPU and the MTRR state of VCPU#0. Do > > you think you could switch this code to use the newly introduced > > machinery to save a single instance of a specific type? > Sure, I will add a tool patch at the end of the series Hi Roger, Is this urgent to be in this series? If not I will add a new patch after it is all in. Thanks, Alex > > > > > > > > > > > > > Signed-off-by: Alexandru Isaila > > > > > > --- > > > Changes since V15: > > > - Moved pause/unpause calls into hvm_save_one() > > > - Re-add the loop in hvm_save_one(). > > > --- > > > xen/arch/x86/domctl.c | 2 -- > > > xen/arch/x86/hvm/save.c | 12 ++-- > > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > > > index 8fbbf3a..cb53980 100644 > > > --- a/xen/arch/x86/domctl.c > > > +++ b/xen/arch/x86/domctl.c > > > @@ -591,12 +591,10 @@ long arch_do_domctl( > > > !is_hvm_domain(d) ) > > > break; > > > > > > -domain_pause(d); > > > ret = hvm_save_one(d, domctl->u.hvmcontext_partial.type, > > > domctl- > > > >u.hvmcontext_partial.instance, > > > domctl->u.hvmcontext_partial.buffer, > > > &domctl->u.hvmcontext_partial.bufsz); > > > -domain_unpause(d); > > > > > > if ( !ret ) > > > copyback = true; > > > diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c > > > index 49741e0..2d35f17 100644 > > > --- a/xen/arch/x86/hvm/save.c > > > +++ b/xen/arch/x86/hvm/save.c > > > @@ -149,12 +149,15 @@ int hvm_save_one(struct domain *d, unsigned > > > int typecode, unsigned int instance, > > > instance >= d->max_vcpus ) > > > return -ENOENT; > > > ctxt.size = hvm_sr_handlers[typecode].size; > > > -if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU ) > > > -ctxt.size *= d->max_vcpus; > > This chunk seems to belong to a different patch? > > > > The change just mentions pausing a vpcu instead of the whole > > domain, > > but the size of the save context doesn't depend on whether the > > whole > > domain is paused vs a single vcpu is paused. > Right, git rebase did not report any error so it tricked me. I will > have that removed from this patch. > > Thanks, > Alex > > ___ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v17 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state
On Mi, 2018-08-29 at 08:13 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 29.08.18 at 16:02, wrote: > > On Mi, 2018-08-22 at 18:15 +0300, Isaila Alexandru wrote: > > > > > > On Mi, 2018-08-22 at 16:41 +0200, Roger Pau Monné wrote: > > > > > > > > If you look at vcpu_hvm in tools/libxc/xc_dom_x86.c it saves > > > > the > > > > full > > > > domain context just to get the CPU and the MTRR state of > > > > VCPU#0. Do > > > > you think you could switch this code to use the newly > > > > introduced > > > > machinery to save a single instance of a specific type? > > > Sure, I will add a tool patch at the end of the series > > Is this urgent to be in this series? If not I will add a new patch > > after it is all in. > Considering the problems that there have been with this series, > anything to help build confidence in things still working for all > cases would help here, so I'm pretty glad Roger thought of this, > and while I wouldn't make it as strong as "the series can't go > in without this", I'd still much prefer if you too the time. I don't think it is possible to use getcontext_partial() in vcpu_hvm() because of the need to have a header for xc_domain_hvm_setcontext() and the only way to get it is by xc_domain_hvm_getcontext(). There is also a comment there that states the same thing "/* * Get the full HVM context in order to have the header, it is not * possible to get the header with getcontext_partial, and crafting one * from userspace is also not an option since cpuid is trapped and * modified by Xen. */ " I hope I understood the request correctly to start with and if not please clarify. Regards, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v17 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state
On Lu, 2018-09-03 at 16:36 +0200, Roger Pau Monné wrote: > On Fri, Aug 31, 2018 at 04:56:21PM +0300, Isaila Alexandru wrote: > > > > On Mi, 2018-08-29 at 08:13 -0600, Jan Beulich wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 29.08.18 at 16:02, wrote: > > > > On Mi, 2018-08-22 at 18:15 +0300, Isaila Alexandru wrote: > > > > > > > > > > > > > > > On Mi, 2018-08-22 at 16:41 +0200, Roger Pau Monné wrote: > > > > > > > > > > > > > > > > > > If you look at vcpu_hvm in tools/libxc/xc_dom_x86.c it > > > > > > saves > > > > > > the > > > > > > full > > > > > > domain context just to get the CPU and the MTRR state of > > > > > > VCPU#0. Do > > > > > > you think you could switch this code to use the newly > > > > > > introduced > > > > > > machinery to save a single instance of a specific type? > > > > > Sure, I will add a tool patch at the end of the series > > > > Is this urgent to be in this series? If not I will add a new > > > > patch > > > > after it is all in. > > > Considering the problems that there have been with this series, > > > anything to help build confidence in things still working for all > > > cases would help here, so I'm pretty glad Roger thought of this, > > > and while I wouldn't make it as strong as "the series can't go > > > in without this", I'd still much prefer if you too the time. > > I don't think it is possible to use getcontext_partial() > > in vcpu_hvm() > > because of the need to have a header for xc_domain_hvm_setcontext() > > and > > the only way to get it is by xc_domain_hvm_getcontext(). There is > > also > > a comment there that states the same thing > > "/* > > * Get the full HVM context in order to have the header, it is > > not > > * possible to get the header with getcontext_partial, and > > crafting > > one > > * from userspace is also not an option since cpuid is trapped > > and > > * modified by Xen. > > */ > > " > > I hope I understood the request correctly to start with and if not > > please clarify. > But I expect you also get such header when fetching the state of a > single device, or else how do you use this new hypercall in > conjunction with xc_domain_hvm_setcontext? > The new *save_one functions are based on the old xc_domain_hvm_getcontext_partial() that did not send the header. I had no requests to change this behavior by this point. Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v18 00/13] x86/domctl: Save info for one vcpu instance
On Fri, 2018-09-07 at 03:48 -0600, Jan Beulich wrote: > > > > On 03.09.18 at 15:14, wrote: > > > > This patch series addresses the ideea of saving data from a single > > vcpu instance. > > First it starts by adding *save_one functions, then it introduces a > > handler for the > > new save_one* funcs and makes use of it in the hvm_save and > > hvm_save_one funcs. > > The final patches are used for clean up and change the > > hvm_save_one() func while > > changing domain_pause to vcpu_pause. > > Considering the ongoing problems with the series, please going > forward indicate here what exact testing you've done to make > sure you introduce no regressions. > I've tested with tools/misc/xen-hvmctx for xc_domain_hvm_getcontext and with tools/xentrace/xenctx for xc_domain_hvm_getcontext_partial. Before this I first looked at the part of the guest startup when it calls xc_domain_hvm_getcontext to save all the info. If there are more tests that I could run please provide. Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v18 12/13] x86/hvm: Remove redundant save functions
On Fri, 2018-09-07 at 03:43 -0600, Jan Beulich wrote: > > > > On 03.09.18 at 15:14, wrote: > > > > This patch removes the redundant save functions and renames the > > save_one* to save. It then changes the domain param to vcpu in the > > save funcs and adapts print messages in order to match the format > > of the > > other save related messages. > > > > Signed-off-by: Alexandru Isaila > > > > --- > > Changes since V17: > > - Refit HVM_REGISTER_SAVE_RESTORE(CPU) > > - Add const to the added struct domain *d > > Excuse me, but how many more times do I need to point out that this > is wrong? Once again for a single-vCPU guest saving the second PIC > instance will become impossible with this. Just to re-iterate: The > check > above has to be limited to just HVMSR_PER_VCPU kind records. Sorry for the misunderstanding of this matter but in v16 I had +if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU && +instance >= d->max_vcpus ) +return -ENOENT; +if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_DOM ) +instance = 0; and this solved the save call for per_dom having instance = 0. Later, in v17 I dropped the PER_DOM if and in v18 I dropped the && PER_VCPU. Now the question is how exactly should I go with this? > > > ctxt.size = hvm_sr_handlers[typecode].size; > > -if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU ) > > -ctxt.size *= d->max_vcpus; > > ctxt.data = xmalloc_bytes(ctxt.size); > > if ( !ctxt.data ) > > return -ENOMEM; > > > > -if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 ) > > +if ( (rv = hvm_sr_handlers[typecode].save(d->vcpu[instance], > > &ctxt)) != 0 ) > > And you need to use d->vcpu[0] for all others here. > I can add a switch (kind) here and for the PER_VCPU case to call save(d->vcpu[instance]) then for PER_DOM case call save(d->vcpu[0]). Is this a good way to go? Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v19 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state
On Mon, 2018-09-10 at 07:25 -0600, Jan Beulich wrote: > > > > On 10.09.18 at 14:36, wrote: > > > > --- a/xen/arch/x86/hvm/save.c > > +++ b/xen/arch/x86/hvm/save.c > > @@ -155,6 +155,11 @@ int hvm_save_one(struct domain *d, unsigned > > int typecode, unsigned int instance, > > if ( !ctxt.data ) > > return -ENOMEM; > > > > +if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU ) > > +vcpu_pause(d->vcpu[instance]); > > Is there any reason why you don't use v here and ... There is not reason but I did not want to modify the reviewed patch further Alex > > +else > > +domain_pause(d); > > + > > if ( (rv = hvm_sr_handlers[typecode].save(v, &ctxt)) != 0 ) > > printk(XENLOG_G_ERR "HVM%d save: failed to save type > > %"PRIu16" (%d)\n", > > d->domain_id, typecode, rv); > > @@ -186,6 +191,11 @@ int hvm_save_one(struct domain *d, unsigned > > int typecode, unsigned int instance, > > } > > } > > > > +if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU ) > > +vcpu_unpause(d->vcpu[instance]); > > ... here? > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v19 12/13] x86/hvm: Remove redundant save functions
On Mon, 2018-09-10 at 15:36 +0300, Alexandru Isaila wrote:
> This patch removes the redundant save functions and renames the
> save_one* to save. It then changes the domain param to vcpu in the
> save funcs and adapts print messages in order to match the format of
> the
> other save related messages.
>
> Signed-off-by: Alexandru Isaila
>
> ---
> Changes since V18:
> - Add const struct domain to rtc_save and hpet_save
> - Latched the vCPU into a local variable in hvm_save_one()
> - Add HVMSR_PER_VCPU kind check to the bounds if.
> ---
> xen/arch/x86/cpu/mcheck/vmce.c | 18 +---
> xen/arch/x86/emul-i8254.c | 5 ++-
> xen/arch/x86/hvm/hpet.c| 7 ++--
> xen/arch/x86/hvm/hvm.c | 75 +++-
> --
> xen/arch/x86/hvm/irq.c | 15 ---
> xen/arch/x86/hvm/mtrr.c| 22 ++
> xen/arch/x86/hvm/pmtimer.c | 5 ++-
> xen/arch/x86/hvm/rtc.c | 5 ++-
> xen/arch/x86/hvm/save.c| 28 +++--
> xen/arch/x86/hvm/vioapic.c | 5 ++-
> xen/arch/x86/hvm/viridian.c| 23 ++-
> xen/arch/x86/hvm/vlapic.c | 38 ++---
> xen/arch/x86/hvm/vpic.c| 5 ++-
> xen/include/asm-x86/hvm/save.h | 8 +---
> 14 files changed, 63 insertions(+), 196 deletions(-)
>
> @@ -141,6 +138,8 @@ int hvm_save_one(struct domain *d, unsigned int
> typecode, unsigned int instance,
> int rv;
> hvm_domain_context_t ctxt = { };
> const struct hvm_save_descriptor *desc;
> +struct vcpu *v = (hvm_sr_handlers[typecode].kind ==
> HVMSR_PER_VCPU) ?
> + d->vcpu[instance] : d->vcpu[0];
>
Sorry for the inconvenience but I've just realized that this has to be
initialize after the bounds check. I will have this in mine
Thanks,
Alex
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v19 12/13] x86/hvm: Remove redundant save functions
On Mon, 2018-09-10 at 07:42 -0600, Jan Beulich wrote:
> > > > On 10.09.18 at 15:33, wrote:
> >
> > On Mon, 2018-09-10 at 15:36 +0300, Alexandru Isaila wrote:
> > > This patch removes the redundant save functions and renames the
> > > save_one* to save. It then changes the domain param to vcpu in
> > > the
> > > save funcs and adapts print messages in order to match the format
> > > of
> > > the
> > > other save related messages.
> > >
> > > Signed-off-by: Alexandru Isaila
> > >
> > > ---
> > > Changes since V18:
> > > - Add const struct domain to rtc_save and hpet_save
> > > - Latched the vCPU into a local variable in hvm_save_one()
> > > - Add HVMSR_PER_VCPU kind check to the bounds if.
> > > ---
> > > xen/arch/x86/cpu/mcheck/vmce.c | 18 +---
> > > xen/arch/x86/emul-i8254.c | 5 ++-
> > > xen/arch/x86/hvm/hpet.c| 7 ++--
> > > xen/arch/x86/hvm/hvm.c | 75 +++-
> > >
> > > --
> > > xen/arch/x86/hvm/irq.c | 15 ---
> > > xen/arch/x86/hvm/mtrr.c| 22 ++
> > > xen/arch/x86/hvm/pmtimer.c | 5 ++-
> > > xen/arch/x86/hvm/rtc.c | 5 ++-
> > > xen/arch/x86/hvm/save.c| 28 +++--
> > > xen/arch/x86/hvm/vioapic.c | 5 ++-
> > > xen/arch/x86/hvm/viridian.c| 23 ++-
> > > xen/arch/x86/hvm/vlapic.c | 38 ++---
> > > xen/arch/x86/hvm/vpic.c| 5 ++-
> > > xen/include/asm-x86/hvm/save.h | 8 +---
> > > 14 files changed, 63 insertions(+), 196 deletions(-)
> > >
> > > @@ -141,6 +138,8 @@ int hvm_save_one(struct domain *d, unsigned
> > > int
> > > typecode, unsigned int instance,
> > > int rv;
> > > hvm_domain_context_t ctxt = { };
> > > const struct hvm_save_descriptor *desc;
> > > +struct vcpu *v = (hvm_sr_handlers[typecode].kind ==
> > > HVMSR_PER_VCPU) ?
> > > + d->vcpu[instance] : d->vcpu[0];
> > >
> >
> > Sorry for the inconvenience but I've just realized that this has to
> > be
> > initialize after the bounds check. I will have this in mine
>
> Also to eliminate redundancy I'd prefer if you moved the conditional
> expression inside the square brackets.
>
Are these changes worth waiting 24h?
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks
On Thu, 2018-09-13 at 08:17 -0600, Jan Beulich wrote:
> > > > On 12.09.18 at 11:47, wrote:
> >
> > The original version of the patch emulated the current instruction
> > (which, as a side-effect, emulated the page-walk as well), however
> > we
> > need finer-grained control. We want to emulate the page-walk, but
> > still
> > get an EPT violation event if the current instruction would trigger
> > one.
> > This patch performs just the page-walk emulation.
>
> Rather than making this basically a revision log, could you please
> focus
> on what you actually want to achieve?
>
> As to the title: "Suppress ..." please.
>
> > @@ -149,6 +151,10 @@ guest_walk_tables(struct vcpu *v, struct
> > p2m_domain *p2m,
> > ar_and &= gflags;
> > ar_or |= gflags;
> >
> > +if ( set_ad && set_ad_bits(&l4p[guest_l4_table_offset(va)].l4,
> > + &gw->l4e.l4, false) )
> > +accessed = true;
>
> It is in particular this seemingly odd (and redundant with what's
> done
> later in the function) which needs thorough explanation.
On this patch I've followed Andrew Cooper's suggestion on how to set
A/D Bits:
"While walking down the levels, set any missing A bits and remember if
we
set any. If we set A bits, consider ourselves complete and exit back
to
the guest. If no A bits were set, and the access was a write (which we
know from the EPT violation information), then set the leaf D bit."
If I misunderstood the comment please clarify.
Thanks,
Alex
>
> > @@ -362,6 +376,13 @@ guest_walk_tables(struct vcpu *v, struct
> > p2m_domain *p2m,
> > */
> > ar = (ar_and & AR_ACCUM_AND) | (ar_or & AR_ACCUM_OR);
> >
> > +if ( set_ad )
> > +{
> > +set_ad_bits(&l1p[guest_l1_table_offset(va)].l1, &gw-
> > >l1e.l1,
> > +(ar & _PAGE_RW) && !accessed &&
> > !guest_wp_enabled(v));
> > +goto out;
> > +}
>
> Why does A bits being set (or not) in non-leaf (possibly, but even
> that's
> inconsistent in your handling) tables have any meaning for whether
> the
> D bit wants setting in the leaf page table? Similarly, why does it
> matter
> whether the accumulated permissions allow writing, irrespective of
> whether the operation was actually a write? Same for CR0.WP.
>
> > --- a/xen/arch/x86/mm/hap/hap.c
> > +++ b/xen/arch/x86/mm/hap/hap.c
> > @@ -768,7 +768,8 @@ static const struct paging_mode
> > hap_paging_real_mode = {
> > .update_cr3 = hap_update_cr3,
> > .update_paging_modes= hap_update_paging_modes,
> > .write_p2m_entry= hap_write_p2m_entry,
> > -.guest_levels = 1
> > +.guest_levels = 1,
> > +.page_walk_set_ad_bits = hap_page_walk_set_ad_bits_2_levels
> > };
>
> Here and elsewhere, please add new hooks next to other hooks,
> not at the end.
>
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -214,7 +214,10 @@ bool p2m_mem_access_check(paddr_t gpa,
> > unsigned long gla,
> > d->arch.monitor.inguest_pagefault_disabled &&
> > npfec.kind != npfec_kind_with_gla ) /* don't send a
> > mem_event */
> > {
> > -hvm_emulate_one_vm_event(EMUL_KIND_NORMAL,
> > TRAP_invalid_op, X86_EVENT_NO_EC);
> > +struct hvm_hw_cpu ctxt;
> > +
> > +hvm_funcs.save_cpu_ctxt(v, &ctxt);
>
> Pretty expensive an operation when all you're after is
> v->arch.hvm.guest_cr[3].
>
> Jan
>
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Save paused cpu ctx
Hello, I want to restart the discussion on dropping the "if ( v->pause_flags & VPF_down )" from hvm_save_cpu_ctxt() and be able to save context in a vcup down state. The content of the ctx could be filled like it is described in Intel SDM, "9.1.1 Processor State After Reset" (https://so ftware.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1- 2abcd-3abcd.pdf page 2996). Is this enough to be up streamed? Regards, Isaila Alexandru ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Save paused cpu ctx
On Wed, 2018-09-19 at 10:41 +0200, Roger Pau Monné wrote: > On Wed, Sep 19, 2018 at 11:21:32AM +0300, Isaila Alexandru wrote: > > Hello, > > > > I want to restart the discussion on dropping the "if ( v- > > >pause_flags & > > VPF_down )" from hvm_save_cpu_ctxt() and be able to save context in > > a > > vcup down state. The content of the ctx could be filled like it is > > described in Intel SDM, "9.1.1 Processor State After Reset" (https: > > //so > > ftware.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol- > > 1- > > 2abcd-3abcd.pdf page 2996). > > If the point is filling the context with the default reset state why > can't this be done by the tools if required? > > It seems like saving the state of down vCPUs is quite pointless since > the state is always going to be the same, and will just increase the > size of the migration stream, but maybe I'm missing something. > The idea behind this is to be able to query a vcpu and have it's state because right now xen does not return any info about a vcpu if it's down. We do not want to save the context but just to get info about it as a result of a query. Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Save paused cpu ctx
On Wed, 2018-09-19 at 03:01 -0600, Jan Beulich wrote: > > > > On 19.09.18 at 10:21, wrote: > > > > I want to restart the discussion on dropping the "if ( v- > > >pause_flags & > > VPF_down )" from hvm_save_cpu_ctxt() and be able to save context in > > a > > vcup down state. The content of the ctx could be filled like it is > > described in Intel SDM, "9.1.1 Processor State After Reset" (https: > > //so > > ftware.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol- > > 1- > > 2abcd-3abcd.pdf page 2996). > > > > Is this enough to be up streamed? > > Along the lines of what Roger has said - without knowing _why_ you > want to do that, it's hard to tell. It's pretty clear I think that we > don't > want to outright drop the condition, the question is just whether to > somewhat relax it for your purposes. > > I'm afraid I also don't agree with your assertion that a down vCPU is > in "after reset" state: To me, such a vCPU could legitimately be > considered to be in that state, in the state it was in when it went > down, or in basically any other (perhaps random) state. Processor State After Reset is just the name of the table in the manual. Having a default state to return on a query was the start point of the discussion. You are right, we can return whatever state the cpu was in before it went down. The code works fine like that, further more, this will make us have a smaller patch plus a legitimate ctx at the end of the query. > Finally (and I'm sorry for being picky here) - why was this inquiry > addressed _To_ me instead of the list? It was addressed to you because in v17 of the save_one series you said that we have to continue the discussion in a different thread. Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] x86/hvm: Change return error for offline vcpus
On Thu, 2018-09-20 at 07:55 -0600, Jan Beulich wrote: > > > > On 20.09.18 at 14:54, wrote: > > > > --- a/xen/arch/x86/hvm/save.c > > +++ b/xen/arch/x86/hvm/save.c > > @@ -165,7 +165,7 @@ int hvm_save_one(struct domain *d, unsigned int > > typecode, unsigned int instance, > > if ( (rv = hvm_sr_handlers[typecode].save(v, &ctxt)) != 0 ) > > printk(XENLOG_G_ERR "HVM%d save: failed to save type > > %"PRIu16" (%d)\n", > > d->domain_id, typecode, rv); > > -else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) ) > > +else if ( rv = -ENODATA, ctxt.cur >= sizeof(*desc) ) > > I think this change of error code should once again only be done > for HVMSR_PER_VCPU kind records. For the others no data > appearing is _the_ indication of the instance not existing. > Ok, I will add a conditional for this inside the if() Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/hvm: Change return error for offline vcpus
On Fri, 2018-09-21 at 09:44 +0100, Wei Liu wrote:
> On Fri, Sep 21, 2018 at 09:43:22AM +0100, Wei Liu wrote:
> > On Fri, Sep 21, 2018 at 10:30:30AM +0300, Alexandru Isaila wrote:
> > > This patch is needed in order to have a different return error
> > > for invalid vcpu
> > > and offline vcpu on the per vcpu king.
> >
> > Sorry, I can't parse this sentence. What is "per vcpu king"?
>
> Oh, "king" should be "kind".
Yes, king = kind, sorry for that
>
> >
> > >
> > > Signed-off-by: Alexandru Isaila
> > >
> > > ---
> > > Changes since V1:
> > > - Add conditional statement in order to have a difference
> > > between
> > > per_vcpu and per_dom return error.
> > > ---
> > > xen/arch/x86/hvm/save.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> > > index d520898843..1764fb0918 100644
> > > --- a/xen/arch/x86/hvm/save.c
> > > +++ b/xen/arch/x86/hvm/save.c
> > > @@ -165,7 +165,8 @@ int hvm_save_one(struct domain *d, unsigned
> > > int typecode, unsigned int instance,
> > > if ( (rv = hvm_sr_handlers[typecode].save(v, &ctxt)) != 0 )
> > > printk(XENLOG_G_ERR "HVM%d save: failed to save type
> > > %"PRIu16" (%d)\n",
> > > d->domain_id, typecode, rv);
> > > -else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
> > > +else if ( rv = hvm_sr_handlers[typecode].kind ==
> > > HVMSR_PER_VCPU ?
> > > + -ENODATA : -ENOENT, ctxt.cur >= sizeof(*desc) )
> > > {
> > > uint32_t off;
> > >
> > > --
> > > 2.17.1
> > >
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/hvm: Change return error for offline vcpus
On Fri, 2018-09-21 at 04:34 -0600, Jan Beulich wrote: > > > > On 21.09.18 at 09:30, wrote: > > > > --- a/xen/arch/x86/hvm/save.c > > +++ b/xen/arch/x86/hvm/save.c > > @@ -165,7 +165,8 @@ int hvm_save_one(struct domain *d, unsigned int > > typecode, unsigned int instance, > > if ( (rv = hvm_sr_handlers[typecode].save(v, &ctxt)) != 0 ) > > printk(XENLOG_G_ERR "HVM%d save: failed to save type > > %"PRIu16" (%d)\n", > > d->domain_id, typecode, rv); > > -else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) ) > > +else if ( rv = hvm_sr_handlers[typecode].kind == > > HVMSR_PER_VCPU ? > > + -ENODATA : -ENOENT, ctxt.cur >= sizeof(*desc) ) > > This very certainly needs parenthesizes, since if asked explicitly I > don't think many people will be able to quickly answer the question > of precedence between the ?: and , operators. I'm happy to add > these while committing, and with them in place > Acked-by: Jan Beulich > Ok, thanks for the help. Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] x86/mm: Add mem access rights to NPT
On Wed, 2018-07-25 at 04:37 -0600, Jan Beulich wrote:
> > > > On 25.07.18 at 11:25, wrote:
> >
> > On 07/24/2018 01:02 PM, Jan Beulich wrote:
> > > > > > On 24.07.18 at 13:26, wrote:
> > > >
> > > > On 07/24/2018 09:55 AM, Jan Beulich wrote:
> > > > > > > > On 23.07.18 at 15:48, wrote:
> > > > > >
> > > > > > +{
> > > > > > +xfree(d->arch.monitor.msr_bitmap);
> > > > > > +return -ENOMEM;
> > > > > > +}
> > > > > > +radix_tree_init(p2m->mem_access_settings);
> > > > > > +}
> > > > >
> > > > > What's the SVM connection here? Please don't forget that p2m-
> > > > > pt.c
> > > > > also serves the shadow case. Perhaps struct p2m_domain should
> > > > > contain a boolean indicator whether this auxiliary data
> > > > > structure is
> > > > > needed?
> > > >
> > > > It's basically just "hap_enabled()" isn't it?
> > >
> > > Only if we can't make it there when EPT is active.
> >
> > It can make it here when VMX is active and shadow is enabled, but
> > it
> > shouldn't be able to get here when EPT is active. We could add an
> > ASSERT() to that effect; it should be safe in production, as the
> > only
> > side effect should be that we do a small pointless allocation.
>
> So I've looked a little more closely: This is being added to
> arch_monitor_init_domain(), called from vm_event_domctl(). I can't
> see why this wouldn't be reachable with EPT enabled.
>
Hi George,
Any input on this?
Regards,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On Wed, 2018-09-26 at 17:47 +0100, George Dunlap wrote: > From: Isaila Alexandru > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT > p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" >extra information > > - Remove entires which match the default extra information rather >than continuing to store them > > - For superpages, only store an entry for the first gfn in the >superpage. Use the order of the p2m entry being read to determine >the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila > Signed-off-by: George Dunlap > --- > NB, this is compile-tested only. I've tested this with xen-access and it works as expected > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index 3c42e21906..2e6b1e75e4 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -20,6 +20,7 @@ > */ > > #include > +#include Is this intended? Regards, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] mem_access: Fix npfec.kind propagation
On Thu, 2018-09-27 at 12:25 +0100, George Dunlap wrote: > The name of the "with_gla" flag is confusing; it has nothing to do > with the existence or lack thereof of a faulting GLA, but rather > where > the fault originated. The npfec.kind value is always valid, and > should thus be propagated, regardless of whether gla_valid is set or > not. > > In particular, gla_valid will never be set on AMD systems; but > npfec.kind will still be valid and should still be propagated. > > Signed-off-by: Alexandru Isaila > Signed-off-by: George Dunlap Reviewed-by: Alexandru Isaila ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6] x86/altp2m: Hypercall to set altp2m view visibility
On 27.03.2020 04:30, Tian, Kevin wrote: From: Isaila Alexandru Sent: Tuesday, March 24, 2020 6:46 PM Hi Kevin and sorry for the long reply time, On 10.03.2020 04:04, sTian, Kevin wrote: From: Alexandru Stefan ISAILA Sent: Tuesday, March 3, 2020 8:23 PM 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. I look forward to more elaboration of the motivation, especially for one who doesn't track altp2m closely like me. For example, do_altp2m_op mentions three modes: external, internal, coordinated. Then is this patch trying to limit the view switch in all three modes or just one of them? from the definition clearly external disallows guest to change any view (then why do we want per-view visibility control) while the latter two both allows guest to switch the view. later you noted some exception with mixed (internal) mode. then is this restriction pushed just for limited (coordinated) mode? As you stated, there are some exceptions with mixed (internal) mode. This restriction is clearly used for coordinated mode but it also restricts view switching in the external mode as well. I had a good example to start with, let's say we have one external agent in dom0 that uses view1 and view2 and the logic requires the switch between the views. At this point VMFUNC is available to the guest so with a simple asm code it can witch to view 0. At this time the external agent is not aware that the view has switched and further more view0 was not supposed to be in the main logic so it crashes. This example can be extended to any number of views. I hope it can paint a more clear picture of what this patch is trying to achive. Can VMFUNC be hidden and disabled when external mode is being used? or is it because the mode can be dynamically switched after a VM is launched so you have to restrict the views in this way? Like you said, there is a problem if the mode is dynamically switched. btw I'm not sure why altp2m invents two names per mode, and their mapping looks a bit weird. e.g. isn't 'coordinated' mode sound more like 'mixed' mode? Yes that is true, it si a bit weird. The new xc_altp2m_set_visibility() solves this by making views invisible to vmfunc. if one doesn't want to make view visible to vmfunc, why can't he just avoids registering the view at the first place? Are you aiming for a scenario that dom0 may register 10 views, with 5 views visible to vmfunc with the other 5 views switched by dom0 itself? That is one scenario, another can be that dom0 has a number of views created and in some time it wants to be sure that only some of the views can be switched, saving the rest and making them visible when the time is right. Sure the example given up is another example. Can you update the patch description and resend? I'll take another look then. Ok, I will update the description for the next version. Thanks, Alex
Re: [PATCH V7] x86/altp2m: Hypercall to set altp2m view visibility
On 31.03.2020 10:43, Jan Beulich wrote: On 30.03.2020 08:54, Alexandru Isaila wrote: 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(). The usage of this hypercall is aimed at dom0 having a logic with a number of views created and at some time there is a need to be sure that only some of the views can be switched, saving the rest and making them visible when the time is right. 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 For v6 I did provide a hypervisor side R-b; I didn't think ... No you didn't. Changes since V6: - Update commit message. ... this alone would have warranted to drop it? I don't think so and if you provide a r-b now I will add it if it will be a need for another version (if nothing big changes). Thanks, Alex
Ping: [PATCH V7] x86/altp2m: Hypercall to set altp2m view visibility
Hi Kevin,
This is a kind reminder if you can have another look at the new version
of this patch.
Thanks,
Alex
On 30.03.2020 09:54, Alexandru Isaila wrote:
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().
The usage of this hypercall is aimed at dom0 having a logic with a number of
views
created and at some time there is a need to be sure that only some of the views
can be switched, saving the rest and making them visible when the time
is right.
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
---
CC: Ian Jackson
CC: Wei Liu
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Julien Grall
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: "Roger Pau Monné"
CC: Jun Nakajima
CC: Kevin Tian
---
Changes since V6:
- Update commit message.
Changes since V5:
- Change idx type from uint16_t to unsigned int
- Add rc var and dropped the err return from p2m_get_suppress_ve().
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 | 34 +++--
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, 108 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index fc6e57a1a0..2e6e652678 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 a3d115b650..375e9cf368 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4511,6 +4511,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:
@@ -4788,6 +4789,19 @@ static int do_altp2m_op(
break;
Re: [PATCH V7] x86/altp2m: Hypercall to set altp2m view visibility
On 10.04.2020 06:10, Tian, Kevin wrote:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a3d115b650..375e9cf368 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4511,6 +4511,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:
@@ -4788,6 +4789,19 @@ static int do_altp2m_op(
break;
}
+case HVMOP_altp2m_set_visibility:
+{
+unsigned int 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));
Is "altp2m_visible_eptp" more accurate here? 'working' is a bit misleading
since even invisible eptp could still work but just not directly togged by
vmfunc...
Yes, you are right and I can change this before it is commited.
otherwise,
Reviewed-by: Kevin Tian
Thanks for the review,
Alex
Re: [PATCH V7] x86/altp2m: Hypercall to set altp2m view visibility
Are there any more r-b needed for this patch?
Thanks,
Alex
On 30.03.2020 09:54, Alexandru Isaila wrote:
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().
The usage of this hypercall is aimed at dom0 having a logic with a number of
views
created and at some time there is a need to be sure that only some of the views
can be switched, saving the rest and making them visible when the time
is right.
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
---
CC: Ian Jackson
CC: Wei Liu
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Julien Grall
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: "Roger Pau Monné"
CC: Jun Nakajima
CC: Kevin Tian
---
Changes since V6:
- Update commit message.
Changes since V5:
- Change idx type from uint16_t to unsigned int
- Add rc var and dropped the err return from p2m_get_suppress_ve().
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 | 34 +++--
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, 108 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index fc6e57a1a0..2e6e652678 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 a3d115b650..375e9cf368 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4511,6 +4511,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:
@@ -4788,6 +4789,19 @@ static int do_altp2m_op(
break;
}
+case HVMOP_altp2m_set_visibility:
+{
+
Ping: [PATCH V8] x86/altp2m: Hypercall to set altp2m view visibility
Hi,
I need a review for the tools bits in this patch.
Thanks,
Alex
On 13.04.2020 09:51, Alexandru Isaila wrote:
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().
The usage of this hypercall is aimed at dom0 having a logic with a number of
views
created and at some time there is a need to be sure that only some of the views
can be switched, saving the rest and making them visible when the time
is right.
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
Reviewed-by: Jan Beulich
Reviewed-by: Kevin Tian
---
CC: Ian Jackson
CC: Wei Liu
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Julien Grall
CC: Stefano Stabellini
CC: "Roger Pau Monné"
CC: Jun Nakajima
CC: Kevin Tian
---
Changes since V7:
- Change altp2m_working_eptp to altp2m_visible_eptp
- Rebase.
Changes since V6:
- Update commit message.
Changes since V5:
- Change idx type from uint16_t to unsigned int
- Add rc var and dropped the err return from p2m_get_suppress_ve().
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 | 34 +++--
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, 108 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 58fa931de1..5f25c5a6d4 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 827c5fa89d..6f6f3f73a8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4509,6 +4509,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;
defau
Re: [Xen-devel] [PATCH] vmevent: reduce include dependencies
On 09.03.2020 13:51, Jan Beulich wrote: There's no need for virtually everything to include public/vm_event.h. Move its inclusion out of sched.h. This requires using the non-typedef name in p2m_mem_paging_resume()'s prototype; by not changing the function definition at the same time it'll remain certain that the build would fail if the typedef itself was changed. Signed-off-by: Jan Beulich Reviewed-by: Alexandru Isaila ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6] x86/altp2m: Hypercall to set altp2m view visibility
Hi Kevin and sorry for the long reply time, On 10.03.2020 04:04, sTian, Kevin wrote: From: Alexandru Stefan ISAILA Sent: Tuesday, March 3, 2020 8:23 PM 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. I look forward to more elaboration of the motivation, especially for one who doesn't track altp2m closely like me. For example, do_altp2m_op mentions three modes: external, internal, coordinated. Then is this patch trying to limit the view switch in all three modes or just one of them? from the definition clearly external disallows guest to change any view (then why do we want per-view visibility control) while the latter two both allows guest to switch the view. later you noted some exception with mixed (internal) mode. then is this restriction pushed just for limited (coordinated) mode? As you stated, there are some exceptions with mixed (internal) mode. This restriction is clearly used for coordinated mode but it also restricts view switching in the external mode as well. I had a good example to start with, let's say we have one external agent in dom0 that uses view1 and view2 and the logic requires the switch between the views. At this point VMFUNC is available to the guest so with a simple asm code it can witch to view 0. At this time the external agent is not aware that the view has switched and further more view0 was not supposed to be in the main logic so it crashes. This example can be extended to any number of views. I hope it can paint a more clear picture of what this patch is trying to achive. btw I'm not sure why altp2m invents two names per mode, and their mapping looks a bit weird. e.g. isn't 'coordinated' mode sound more like 'mixed' mode? Yes that is true, it si a bit weird. The new xc_altp2m_set_visibility() solves this by making views invisible to vmfunc. if one doesn't want to make view visible to vmfunc, why can't he just avoids registering the view at the first place? Are you aiming for a scenario that dom0 may register 10 views, with 5 views visible to vmfunc with the other 5 views switched by dom0 itself? That is one scenario, another can be that dom0 has a number of views created and in some time it wants to be sure that only some of the views can be switched, saving the rest and making them visible when the time is right. Sure the example given up is another example. Regards, Alex
Re: [PATCH RFC v2 8/8] evtchn: don't call Xen consumer callback with per-channel lock held
Hi Jan and sorry for the late reply,
On 20.10.2020 17:13, Jan Beulich wrote:
While there don't look to be any problems with this right now, the lock
order implications from holding the lock can be very difficult to follow
(and may be easy to violate unknowingly). The present callbacks don't
(and no such callback should) have any need for the lock to be held.
Signed-off-by: Jan Beulich
---
TODO: vm_event_disable() frees the structures used by respective
callbacks - need to either use call_rcu() for freeing, or maintain
a count of in-progress calls, for evtchn_close() to wait to drop
to zero before dropping the lock / returning.
I would go with the second solution and maintain a count of in-progress
calls.
Tamas, Petre how does this sound?
Alex
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -763,9 +763,18 @@ int evtchn_send(struct domain *ld, unsig
rport = lchn->u.interdomain.remote_port;
rchn = evtchn_from_port(rd, rport);
if ( consumer_is_xen(rchn) )
-xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], rport);
-else
-evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
+{
+/* Don't keep holding the lock for the call below. */
+xen_event_channel_notification_t fn = xen_notification_fn(rchn);
+struct vcpu *rv = rd->vcpu[rchn->notify_vcpu_id];
+
+rcu_lock_domain(rd);
+spin_unlock_irqrestore(&lchn->lock, flags);
+fn(rv, rport);
+rcu_unlock_domain(rd);
+return 0;
+}
+evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
break;
case ECS_IPI:
evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
Re: [PATCH v3 5/5] evtchn: don't call Xen consumer callback with per-channel lock held
On 23.11.2020 15:30, Jan Beulich wrote:
While there don't look to be any problems with this right now, the lock
order implications from holding the lock can be very difficult to follow
(and may be easy to violate unknowingly). The present callbacks don't
(and no such callback should) have any need for the lock to be held.
However, vm_event_disable() frees the structures used by respective
callbacks and isn't otherwise synchronized with invocations of these
callbacks, so maintain a count of in-progress calls, for evtchn_close()
to wait to drop to zero before freeing the port (and dropping the lock).
Signed-off-by: Jan Beulich
Reviewed-by: Alexandru Isaila
---
Should we make this accounting optional, to be requested through a new
parameter to alloc_unbound_xen_event_channel(), or derived from other
than the default callback being requested?
---
v3: Drain callbacks before proceeding with closing. Re-base.
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -397,6 +397,7 @@ static long evtchn_bind_interdomain(evtc
rchn->u.interdomain.remote_dom = ld;
rchn->u.interdomain.remote_port = lport;
+atomic_set(&rchn->u.interdomain.active_calls, 0);
rchn->state = ECS_INTERDOMAIN;
/*
@@ -720,6 +721,10 @@ int evtchn_close(struct domain *d1, int
double_evtchn_lock(chn1, chn2);
+if ( consumer_is_xen(chn1) )
+while ( atomic_read(&chn1->u.interdomain.active_calls) )
+cpu_relax();
+
evtchn_free(d1, chn1);
chn2->state = ECS_UNBOUND;
@@ -781,9 +786,15 @@ int evtchn_send(struct domain *ld, unsig
rport = lchn->u.interdomain.remote_port;
rchn = evtchn_from_port(rd, rport);
if ( consumer_is_xen(rchn) )
+{
+/* Don't keep holding the lock for the call below. */
+atomic_inc(&rchn->u.interdomain.active_calls);
+evtchn_read_unlock(lchn);
xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], rport);
-else
-evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
+atomic_dec(&rchn->u.interdomain.active_calls);
+return 0;
+}
+evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
break;
case ECS_IPI:
evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -104,6 +104,7 @@ struct evtchn
} unbound; /* state == ECS_UNBOUND */
struct {
evtchn_port_t remote_port;
+atomic_t active_calls;
struct domain *remote_dom;
} interdomain; /* state == ECS_INTERDOMAIN */
struct {
Re: [PATCH v2 09/12] x86: make mem-paging configuarable and default it to off for being unsupported
On 12.04.2021 17:12, Jan Beulich wrote: CAUTION: This email originated from outside of our organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. While doing so, make the option dependent upon HVM, which really is the main purpose of the change. Signed-off-by: Jan Beulich Reviewed-by: Alexandru Isaila
Re: [PATCH v2 09/12] x86: make mem-paging configuarable and default it to off for being unsupported
On 12.04.2021 17:18, Tamas K Lengyel wrote: CAUTION: This email originated from outside of our organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. On Mon, Apr 12, 2021 at 10:12 AM Jan Beulich wrote: While doing so, make the option dependent upon HVM, which really is the main purpose of the change. IMHO it would be better to just remove this dead code altogether. I agree with Tamas here. Alex
Re: [PATCH v2 08/12] x86: mem-access is HVM-only
On 12.04.2021 17:10, Jan Beulich wrote: CAUTION: This email originated from outside of our organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. By excluding the file from being built for !HVM, #ifdef-ary can be removed from it. The new HVM dependency on the Kconfig option is benign for Arm. Signed-off-by: Jan Beulich Reviewed-by: Alexandru Isaila
