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

["Natarajan, Janakarajan" <jnataraj@xxxxxxx>]

On 5/16/2018 10:29 AM, Jan Beulich wrote:
> > > > 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.

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