Re: [PATCH v1 04/28] x86/hvm: avic: add domain state initialization

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 05/11/2026 à 02:37, dmukhin@xxxxxxxx a écrit :
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Establish per-domain SVM AVIC backing page mapping and SPA mappings
> for APIC physical/logical ID tables. Use MEMF_no_refcount mappings
> similarly to Intel's VMX APICv setup.
>
> Gate per-domain AVIC state initialization behind 'svm_avic=' command
> line option.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
>   xen/arch/x86/hvm/svm/Makefile            |   1 +
>   xen/arch/x86/hvm/svm/avic.c              | 136 +++++++++++++++++++++++
>   xen/arch/x86/hvm/svm/avic.h              |  14 +++
>   xen/arch/x86/hvm/svm/svm.c               |  20 ++++
>   xen/arch/x86/include/asm/hvm/svm-types.h |   8 ++
>   5 files changed, 179 insertions(+)
>   create mode 100644 xen/arch/x86/hvm/svm/avic.c
>   create mode 100644 xen/arch/x86/hvm/svm/avic.h
>
> diff --git a/xen/arch/x86/hvm/svm/Makefile b/xen/arch/x86/hvm/svm/Makefile
> index 8a072cafd572..7dd16e32a77d 100644
> --- a/xen/arch/x86/hvm/svm/Makefile
> +++ b/xen/arch/x86/hvm/svm/Makefile
> @@ -1,4 +1,5 @@
>   obj-y += asid.o
> +obj-y += avic.o
>   obj-y += emulate.o
>   obj-bin-y += entry.o
>   obj-y += intr.o
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> new file mode 100644
> index 000000000000..2c0e113ae34e
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AMD Advanced Virtual Interrupt Controller (AVIC)
> + */
> +
> +#include <asm/hvm/emulate.h>
> +#include <asm/msr.h>
> +#include <asm/p2m.h>
> +#include <xen/param.h>
> +
> +#include "avic.h"
> +
> +bool __read_mostly svm_avic_enable;
> +
We probably want it to be __ro_after_init, I don't think anything good 
can happen if it's toggled dynamically.
> +static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
> +    avic_apic_access_page[PAGE_SIZE];
> +
I'm a bit dubious in making this const.

I also think guests can find a way to read/write to it (as it's mapped 
as MMIO direct in p2m), which could be a problem regardless of const-ness.
I guess we're supposed to map the vAPIC backing page into the guest 
(instead of a stub page), even if it's not the page used by AVIC hardware.
> System software is responsible for setting up a translation in the 
nested page table granting guest read and write permissions for accesses 
to the vAPIC Backing Page in SPA space. AVIC hardware walks the nested 
page table to check permissions, but does not use the SPA address 
specified in the leaf page table entry. Instead, AVIC hardware finds 
this address in the AVIC_BACKING_PAGE pointer field of the VMCB.
> +static int cf_check avic_parse_cmdline(const char *s)
> +{
> +    int rc;
> +
> +    rc = parse_bool(s, NULL);
> +    if ( rc != -1 )
> +    {
> +        svm_avic_enable = !!rc;
> +    }
> +    else if ( *s == '\0' )
> +    {
> +        svm_avic_enable = true;
> +    }
> +    else
> +    {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +custom_param("svm_avic", avic_parse_cmdline);
> +
> +/*
> + * Enable guest accesses to APIC_DEFAULT_PHYS_BASE.
> + *
> + * AVIC walks the nested page table to check access permissions, but
> + * ignores the SPA from the leaf entry and instead uses the address in
> + * VMCB.avic_backing_page.
> + *
> + * Install a mapping so APIC accesses don't trap to Xen while xAPIC is
> + * accelerated.
> + */
> +static int avic_set_mmio_p2m_entry(struct domain *d)
> +{
> +    gfn_t gfn = _gfn(paddr_to_pfn(APIC_DEFAULT_PHYS_BASE));
_gfn(paddr_to_pfn(x)) can be simplified as gaddr_to_gfn(x)

> +    mfn_t mfn = _mfn(virt_to_mfn(avic_apic_access_page));
> +
> +    return map_mmio_regions(d, gfn, 1, mfn);
> +}
> +
> +static int avic_clear_mmio_p2m_entry(struct domain *d)
> +{
> +    gfn_t gfn = _gfn(paddr_to_pfn(APIC_DEFAULT_PHYS_BASE));
> +    mfn_t mfn = _mfn(virt_to_mfn(avic_apic_access_page));
> +
> +    return unmap_mmio_regions(d, gfn, 1, mfn);
> +}
> +
> +int svm_avic_domain_initialise(struct domain *d)
> +{
> +    struct svm_domain *sd = &d->arch.hvm.svm;
> +    struct page_info *pg;
> +    mfn_t mfn;
> +
> +    if ( !svm_avic_enable || !has_vlapic(d) )
> +        return 0;
> +
> +    /* AVIC logical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_refcount);
> +    if ( !pg )
> +        return -ENOMEM;
> +
> +    mfn = page_to_mfn(pg);
> +    clear_domain_page(mfn);
> +
> +    sd->avic_logical_id_table_mfn = mfn;
> +    sd->avic_logical_id_table = map_domain_page_global(mfn);
> +
> +    /* AVIC physical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_refcount);
> +    if ( !pg )
> +        return -ENOMEM;
> +
> +    mfn = page_to_mfn(pg);
> +    clear_domain_page(mfn);
> +
> +    sd->avic_physical_id_table_mfn = mfn;
> +    sd->avic_physical_id_table = map_domain_page_global(mfn);
> +
> +    return avic_set_mmio_p2m_entry(d);
> +}
> +
> +void svm_avic_domain_relinquish_resources(struct domain *d)
> +{
> +    struct svm_domain *sd = &d->arch.hvm.svm;
> +    mfn_t mfn;
> +
> +    (void)avic_clear_mmio_p2m_entry(d);
> +
> +    mfn = sd->avic_physical_id_table_mfn;
> +    if ( !mfn_eq(mfn, _mfn(0)) )
> +    {
> +        unmap_domain_page_global(sd->avic_physical_id_table);
> +        sd->avic_physical_id_table = NULL;
> +
> +        free_domheap_page(mfn_to_page(mfn));
> +        sd->avic_physical_id_table_mfn = _mfn(0);
> +    }
> +
> +    mfn = sd->avic_logical_id_table_mfn;
> +    if ( !mfn_eq(mfn, _mfn(0)) )
> +    {
> +        unmap_domain_page_global(sd->avic_logical_id_table);
> +        sd->avic_logical_id_table = NULL;
> +
> +        free_domheap_page(mfn_to_page(mfn));
> +        sd->avic_logical_id_table_mfn = _mfn(0);
> +    }
I think using INVALID_MFN would be better than _mfn(0). But given that 
Intel VMX does also use _mfn(0), I would like other's opinion on that 
matter.
(though, if we switch to page_info *, that may not matter anymore)

> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/hvm/svm/avic.h b/xen/arch/x86/hvm/svm/avic.h
> new file mode 100644
> index 000000000000..3da62f59b0cf
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AMD Advanced Virtual Interrupt Controller (AVIC)
> + */
> +
> +#ifndef __SVM_AVIC_H__
> +#define __SVM_AVIC_H__
> +
> +extern bool svm_avic_enable;
> +
> +int svm_avic_domain_initialise(struct domain *d);
> +void svm_avic_domain_relinquish_resources(struct domain *d);
> +
> +#endif /* __SVM_AVIC_H__ */
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index db81dad4b681..8b7bfed0049f 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -40,6 +40,7 @@
>   
>   #include <public/sched.h>
>   
> +#include "avic.h"
>   #include "nestedhvm.h"
>   #include "svm.h"
>   #include "vmcb.h"
> @@ -1084,6 +1085,11 @@ static int cf_check svm_domain_initialise(struct domain 
> *d)
>           .to   = svm_ctxt_switch_to,
>           .tail = svm_do_resume,
>       };
> +    int rc;
> +
> +    rc = svm_avic_domain_initialise(d);
> +    if ( rc )
> +        return rc;
>   
>       d->arch.ctxt_switch = &csw;
>   
> @@ -1095,6 +1101,11 @@ static int cf_check svm_domain_initialise(struct domain *d)
>       return 0;
>   }
>   
> +static void cf_check svm_domain_relinquish_resources(struct domain *d)
> +{
> +    svm_avic_domain_relinquish_resources(d);
> +}
> +
>   static int cf_check svm_vcpu_initialise(struct vcpu *v)
>   {
>       int rc;
> @@ -2384,6 +2395,7 @@ static struct hvm_function_table __initdata_cf_clobber 
> svm_function_table = {
>       .cpu_up               = svm_cpu_up,
>       .cpu_down             = svm_cpu_down,
>       .domain_initialise    = svm_domain_initialise,
> +    .domain_relinquish_resources = svm_domain_relinquish_resources,
>       .vcpu_initialise      = svm_vcpu_initialise,
>       .vcpu_destroy         = svm_vcpu_destroy,
>       .save_cpu_ctxt        = svm_save_vmcb_ctxt,
> @@ -2464,6 +2476,12 @@ static void __init print_svm_features(void)
>           printk(" - none\n");
>   }
>   
> +static void __init svm_avic_start(void)
> +{
> +    if ( !cpu_has_svm_avic || pv_shim )
> +        svm_avic_enable = false;
What's special about pv_shim ?

> +}
> +
>   const struct hvm_function_table * __init start_svm(void)
>   {
>       svm_host_osvw_reset();
> @@ -2487,6 +2505,8 @@ const struct hvm_function_table * __init start_svm(void)
>       if ( cpu_has_tsc_ratio )
>           svm_function_table.tsc_scaling.ratio_frac_bits = 32;
>   
> +    svm_avic_start();
> +
>       svm_function_table.caps.hap = cpu_has_svm_npt;
>       svm_function_table.caps.hap_superpage_2mb = true;
>       svm_function_table.caps.hap_superpage_1gb = cpu_has_page1gb;
> diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h 
> b/xen/arch/x86/include/asm/hvm/svm-types.h
> index 71427b3c11d1..30bdaf27a856 100644
> --- a/xen/arch/x86/include/asm/hvm/svm-types.h
> +++ b/xen/arch/x86/include/asm/hvm/svm-types.h
> @@ -15,6 +15,14 @@ struct svm_domain {
>               uint64_t status;
>           };
>       } osvw;
> +
> +    /*
> +     * Domain AVIC state.
> +     */
> +    mfn_t avic_logical_id_table_mfn;
> +    uint32_t *avic_logical_id_table;
> +    mfn_t avic_physical_id_table_mfn;
> +    uint64_t *avic_physical_id_table;
I suggest moving avic-specific fields to a dedicated structure e.g 
struct svm_avic (that would allow you to drop 'avic_' prefix for all 
fields).
Is there a reason to use mfn_t instead of page_info * ?

>   };
>   
>   struct svm_vcpu {
Teddy

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature