Re: [Xen-devel] [PATCH 2/8] x86/HVM/SVM: Add AVIC initialization code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.04.18 at 01:01, <Janakarajan.Natarajan@xxxxxxx> wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> 
> Introduce AVIC base initialization code. This includes:
>     * Setting up per-VM data structures.
>     * Setting up per-vCPU data structure.
>     * Initializing AVIC-related VMCB bit fields.
> 
> This patch also introduces a new Xen parameter (svm-avic),
> which can be used to enable/disable AVIC support.

This sentence looks stale/misplaced now.

> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,191 @@
> +/*
> + * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) 
> support
> + * Copyright (c) 2016, Advanced Micro Devices, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/domain_page.h>
> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <asm/acpi.h>
> +#include <asm/apicdef.h>
> +#include <asm/atomic.h>
> +#include <asm/event.h>
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/support.h>
> +#include <asm/hvm/svm/avic.h>
> +#include <asm/hvm/vlapic.h>
> +#include <asm/p2m.h>
> +#include <asm/page.h>
> +
> +/*
> + * Note: Current max index allowed for physical APIC ID table is 255.
> + */

This is a single line comment.

> +#define AVIC_PHY_APIC_ID_MAX    0xFF

Is this really an AVIC-specific constant, rather than e.g.
GET_xAPIC_ID(APIC_ID_MASK)?

> +#define AVIC_VAPIC_BAR_MASK     (((1ULL << 40) - 1) << PAGE_SHIFT)
> +
> +/*
> + * Note:
> + * Currently, svm-avic mode is not supported with nested virtualization.
> + * Therefore, it is not yet currently enabled by default. Once the support
> + * is in-place, this should be enabled by default.
> + */
> +bool svm_avic = 0;

false (or simply omit the intializer)

> +int svm_avic_dom_init(struct domain *d)
> +{
> +    int ret = 0;
> +    struct page_info *pg;
> +
> +    if ( !svm_avic || !has_vlapic(d) )
> +        return 0;
> +
> +    /*
> +     * Note:
> +     * AVIC hardware walks the nested page table to check permissions,
> +     * but does not use the SPA address specified in the leaf page
> +     * table entry since it uses  address in the AVIC_BACKING_PAGE pointer
> +     * field of the VMCB. Therefore, we set up a dummy page for APIC _mfn(0).
> +     */
> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> +                       _mfn(0), PAGE_ORDER_4K,
> +                       p2m_get_hostp2m(d)->default_access);

The use of MFN 0 here looks risky to me: How do you guarantee nothing else
might ever use that P2M entry?

> +    /* Init AVIC logical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    if ( !pg )
> +    {
> +        ret = -ENOMEM;
> +        goto err_out;
> +    }
> +    clear_domain_page(_mfn(page_to_mfn(pg)));
> +    d->arch.hvm_domain.svm.avic_logical_id_table_pg = pg;
> +    d->arch.hvm_domain.svm.avic_logical_id_table = 
> __map_domain_page_global(pg);

Both here and ...

> +    /* Init AVIC physical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    if ( !pg )
> +    {
> +        ret = -ENOMEM;
> +        goto err_out;
> +    }
> +    clear_domain_page(_mfn(page_to_mfn(pg)));
> +    d->arch.hvm_domain.svm.avic_physical_id_table_pg = pg;
> +    d->arch.hvm_domain.svm.avic_physical_id_table = 
> __map_domain_page_global(pg);

... here: Do you really need to store both page pointer and map address?

> +void svm_avic_dom_destroy(struct domain *d)
> +{
> +    if ( !svm_avic || !has_vlapic(d) )
> +        return;
> +
> +    if ( d->arch.hvm_domain.svm.avic_physical_id_table )
> +    {
> +        
> unmap_domain_page_global(d->arch.hvm_domain.svm.avic_physical_id_table);
> +        free_domheap_page(d->arch.hvm_domain.svm.avic_physical_id_table_pg);
> +        d->arch.hvm_domain.svm.avic_physical_id_table_pg = 0;
> +        d->arch.hvm_domain.svm.avic_physical_id_table = 0;

DYM NULL?

> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> +    u32 apic_id;
> +    unsigned long tmp;
> +    struct arch_svm_struct *s = &v->arch.hvm_svm;
> +    struct vmcb_struct *vmcb = s->vmcb;
> +    struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> +    const struct vlapic *vlapic = vcpu_vlapic(v);
> +    struct avic_physical_id_entry *entry = (struct avic_physical_id_entry 
> *)&tmp;
> +
> +    if ( !svm_avic || !has_vlapic(v->domain) )
> +        return 0;
> +
> +    if ( !vlapic || !vlapic->regs_page )
> +        return -EINVAL;
> +
> +    apic_id = vlapic_read_aligned(vcpu_vlapic(v), APIC_ID);
> +    s->avic_last_phy_id = avic_get_physical_id_entry(d, 
> GET_xAPIC_ID(apic_id));
> +    if ( !s->avic_last_phy_id )
> +        return -EINVAL;
> +
> +    vmcb->avic_bk_pg_pa = page_to_maddr(vlapic->regs_page);
> +    vmcb->avic_logical_id_table_pa = 
> domain_page_map_to_mfn(d->avic_logical_id_table) << PAGE_SHIFT;
> +    vmcb->avic_physical_id_table_pa = 
> domain_page_map_to_mfn(d->avic_physical_id_table) << PAGE_SHIFT;

DYM something like mfn_to_maddr()? Please don't open-code things.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel