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

["Natarajan, Janakarajan" <jnataraj@amd.com>]

On 5/16/2018 10:29 AM, Jan Beulich wrote:
>>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> 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.


AVIC hardware uses this page to look at permission bits. AFAIK, nothing 
writes to this page.


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


I'm not aware of systems that don't have permission bits.


>
>> +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?


Wouldn't the logical and physical APIC ID table pages be connected to 
the domain and its heap?


>
>> +    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).

Okay.


>
>> +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()?

Yes. That should 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.

I can remove this from the struct.


>
>> +    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.

Okay. I can change it.

Thanks,
Janak

>
>> +    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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel