Re: [Xen-devel] [PATCH v2 04/10] x86/HVM/SVM: Add AVIC initialization code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@xxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,190 @@
> +/*
> + * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) 
> support
> + * Copyright (c) 2018, 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>

Are all of these really needed? For example, xen/stdbool.h isn't commonly
included by non-header files, but is instead obtained from xen/types.h. That
header, in turn, is rarely required to be included explicitly by non-headers
because almost every header already includes it anyway.

In some cases I'm also not convinced you really mean asm/ (rather than
xen/).

> +/* Note: Current max index allowed for physical APIC ID table is 255. */
> +#define AVIC_PHY_APIC_ID_MAX    GET_xAPIC_ID(APIC_ID_MASK)

I think it was pointed out before that "max" generally means the last valid
value, rather than the first invalid one.

> +/*
> + * 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 = false;
> +
> +static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
> +    avic_backing_page[PAGE_SIZE];

So nothing ever writes to this page? I think it would be misleading if CPU side
writes were possible, yet this was marked const.

Also - does this really need allocating statically (rather than just on systems
actually needing it)?

> +static struct avic_physical_id_entry*
> +avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)

I think the first parameter could be const.

> +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.
> +     */
> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> +                       _mfn(virt_to_mfn(avic_backing_page)), PAGE_ORDER_4K,
> +                       p2m_access_rw);
> +
> +    /* Init AVIC logical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_owner);

Do you really mean d here (and below) rather than NULL?

> +    if ( !pg )
> +    {
> +        ret = -ENOMEM;
> +        goto err_out;
> +    }
> +    clear_domain_page(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);

I think I have said before that I don't think you need to store both
virtual and physical address here, unless both are used frequently.
You establishing a global mapping suggests to me that it's the
virtual address you want to store (MFN and hence struct page_info
can be derived from the mapping via domain_page_map_to_mfn(),
like you already do further down).

> +bool svm_avic_vcpu_enabled(const struct vcpu *v)
> +{
> +    const struct arch_svm_struct *s = &v->arch.hvm_svm;
> +    const struct vmcb_struct *vmcb = s->vmcb;
> +
> +    return vmcb->_vintr.fields.avic_enable;

Please don't use excess local variables (both of them are used just once,
and I'm sure you could get away with just one of the two [or none at all]
without breaking the line length limit).

Also shouldn't this be vmcb_get_vintr()?

> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> +    u32 apic_id;
> +    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;
> +
> +    if ( !svm_avic || !has_vlapic(v->domain) )
> +        return 0;
> +
> +    if ( !vlapic || !vlapic->regs_page )
> +        return -EINVAL;
> +
> +    apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);

Why can't this be vlapic_get_reg()?

> +    s->avic_last_phy_id = avic_get_physical_id_entry(d, 
> GET_xAPIC_ID(apic_id));

You don't appear to read this value outside of this function. Please store
values in struct domain / struct vcpu only if you in fact read them, and
if their calculation isn't trivial.

I also don't appear to understand the purpose of the "last" in the name.

> +    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 = 
> mfn_to_maddr(domain_page_map_to_mfn(d->avic_logical_id_table));
> +    vmcb->avic_physical_id_table_pa = 
> mfn_to_maddr(domain_page_map_to_mfn(d->avic_physical_id_table));
> +
> +    /* Set Physical ID Table Pointer [7:0] to max apic id of the domain */
> +    vmcb->avic_physical_id_table_pa |= (v->domain->max_vcpus * 2) & 0xFF;
> +
> +    entry = s->avic_last_phy_id;
> +    entry->bk_pg_ptr_mfn = (vmcb->avic_bk_pg_pa) >> PAGE_SHIFT;

Please don't open-code paddr_to_pfn() / maddr_to_mfn().

> @@ -215,6 +216,8 @@ static int construct_vmcb(struct vcpu *v)
>              vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
>      }
>  
> +    svm_avic_init_vmcb(v);

This function may fail.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1597,6 +1597,10 @@ int vlapic_init(struct vcpu *v)
>  
>      if (vlapic->regs_page == NULL)
>      {
> +        /*
> +         * SVM AVIC depends on the vlapic->regs_page being a full
> +         * page allocation as it is also used for vAPIC backing page.
> +         */
>          vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);

I'm not convinced of the utility of this comment - iirc the same is true on the
VMX side (and there was no similar comment added here at the time).

> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -0,0 +1,39 @@
> +#ifndef _SVM_AVIC_H_
> +#define _SVM_AVIC_H_
> +
> +#include <xen/compiler.h>

You mean xen/types.h here, or else ...

> +enum avic_incmp_ipi_err_code {
> +    AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE,
> +    AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN,
> +    AVIC_INCMP_IPI_ERR_INV_TARGET,
> +    AVIC_INCMP_IPI_ERR_INV_BK_PAGE,
> +};
> +
> +typedef union avic_logical_id_entry {
> +    u32 raw;

... u32 (which really should be uint32_t - please replace thoughout the series)
may not be available here.

> +    struct __packed {
> +        u32 guest_phy_apic_id : 8;
> +        u32 res               : 23;
> +        u32 valid             : 1;
> +    };
> +} avic_logical_id_entry_t;
> +
> +struct __packed avic_physical_id_entry {
> +        u64 host_phy_apic_id  : 8;
> +        u64 res1              : 4;
> +        u64 bk_pg_ptr_mfn     : 40;
> +        u64 res2              : 10;
> +        u64 is_running        : 1;
> +        u64 valid             : 1;
> +};
> +
> +extern bool svm_avic;
> +
> +int svm_avic_dom_init(struct domain *d);
> +void svm_avic_dom_destroy(struct domain *d);
> +
> +bool svm_avic_vcpu_enabled(const struct vcpu *v);
> +int svm_avic_init_vmcb(struct vcpu *v);

These declarations even need xen/sched.h in place of (or together with)
xen/types.h.

Jan


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