lkml.org 
[lkml]   [2021]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v3 3/3] KVM: SVM: Extend host physical APIC ID field to support more than 8-bit
    On Mon, Dec 13, 2021, Suravee Suthikulpanit wrote:
    > The AVIC physical APIC ID table entry contains the host physical
    > APIC ID field, which the hardware uses to keep track of where each
    > vCPU is running. Originally, the field is an 8-bit value, which can
    > only support physical APIC ID up to 255.
    >
    > To support system with larger APIC ID, the AVIC hardware extends
    > this field to support up to the largest possible physical APIC ID
    > available on the system.
    >
    > Therefore, replace the hard-coded mask value with the value
    > calculated from the maximum possible physical APIC ID in the system.

    ...

    > +static void avic_init_host_physical_apicid_mask(void)
    > +{
    > + if (!x2apic_mode) {
    > + /* If host is in xAPIC mode, default to only 8-bit mask. */
    > + avic_host_physical_id_mask = 0xffULL;

    Use GENMASK(7:0) instead of open coding the equivalent. Oh, and
    avic_host_physical_id_mask doesn't need to be a u64, it's hard capped at 12 bits
    and so can be a simple int.

    > + } else {
    > + u32 count = get_count_order(apic_get_max_phys_apicid());
    > +
    > + avic_host_physical_id_mask = BIT_ULL(count) - 1;
    > + }

    Why is the "legal" mask dynamically calculated? That's way more complicated and
    convoluted then this needs to be.

    The cover letter says

    However, newer AMD systems can have physical APIC ID larger than 255,
    and AVIC hardware has been extended to support upto the maximum physical
    APIC ID available in the system.

    and newer versions of the APM have bits

    11:8 - Reserved/SBZ for legacy APIC; extension of Host Physical APIC ID when
    x2APIC is enabled.
    7:0 - Host Physical APIC ID Physical APIC ID of the physical core allocated by
    the VMM to host the guest virtual processor. This field is not valid
    unless the IsRunning bit is set.

    whereas older versions have

    11:8 - Reserved, SBZ. Should always be set to zero.

    That implies that an APIC ID > 255 on older hardware what ignores bits 11:8 even
    in x2APIC will silently fail, and the whole point of this mask is to avoid exactly
    that.

    To further confuse things, the APM was only partially updated and needs to be fixed,
    e.g. "Figure 15-19. Physical APIC Table in Memory" and the following blurb wasn't
    updated to account for the new x2APIC behavior.

    But at least one APM blurb appears to have been wrong (or the architecture is broken)
    prior to the larger AVIC width:

    Since a destination of FFh is used to specify a broadcast, physical APIC ID FFh
    is reserved.

    We have Rome systems with 256 CPUs and thus an x2APIC ID for a CPU of FFh. So
    either the APM is wrong or AVIC is broken on older large systems.

    Anyways, for the new larger mask, IMO dynamically computing the mask based on what
    APIC IDs were enumerated to the kernel is pointless. If the AVIC doesn't support
    using bits 11:0 to address APIC IDs then KVM is silently hosed no matter what if
    any APIC ID is >255.

    Ideally, there would be a feature flag enumerating the larger AVIC support so we
    could do:

    if (!x2apic_mode || !boot_cpu_has(X86_FEATURE_FANCY_NEW_AVIC))
    avic_host_physical_id_mask = GENMASK(7:0);
    else
    avic_host_physical_id_mask = GENMASK(11:0);

    but since it sounds like that's not the case, and presumably hardware is smart
    enough not to assign APIC IDs it can't address, this can simply be

    if (!x2apic_mode)
    avic_host_physical_id_mask = GENMASK(7:0);
    else
    avic_host_physical_id_mask = GENMASK(11:0);

    and patch 01 to add+export apic_get_max_phys_apicid() goes away.

    > + pr_debug("Using AVIC host physical APIC ID mask %#0llx\n",
    > + avic_host_physical_id_mask);
    > +}
    > +
    > int avic_vm_init(struct kvm *kvm)
    > {
    > unsigned long flags;
    > @@ -943,22 +959,17 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
    > void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
    > {
    > u64 entry;
    > - /* ID = 0xff (broadcast), ID > 0xff (reserved) */
    > int h_physical_id = kvm_cpu_get_apicid(cpu);
    > struct vcpu_svm *svm = to_svm(vcpu);
    >
    > - /*
    > - * Since the host physical APIC id is 8 bits,
    > - * we can support host APIC ID upto 255.
    > - */
    > - if (WARN_ON(h_physical_id > AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
    > + if (WARN_ON(h_physical_id > avic_host_physical_id_mask))

    Not really your code, but this should really be

    if (WARN_ON((h_physical_id & avic_host_physical_id_mask) != h_physical_id))
    return;

    otherwise a negative value will get a false negative.

    LOL, and with respect to FFh being reserved, the current KVM code doesn't treat
    it as reserved. Which is probably a serendipituous bug as it allows addressing
    APID ID FFh when x2APIC is enabled. I suspect we also want:

    if (WARN_ON(!x2apic_mode && h_physical_id == 0xff))
    return;

    > return;
    >
    > entry = READ_ONCE(*(svm->avic_physical_id_cache));
    > WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
    >
    > - entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
    > - entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
    > + entry &= ~avic_host_physical_id_mask;
    > + entry |= h_physical_id;
    >
    > entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
    > if (svm->avic_is_running)
    > @@ -1018,6 +1029,7 @@ bool avic_hardware_setup(bool avic)
    > return false;
    >
    > pr_info("AVIC enabled\n");
    > + avic_init_host_physical_apicid_mask();
    > amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
    > return true;
    > }
    > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
    > index 3fa975031dc9..bbe2fb226b52 100644
    > --- a/arch/x86/kvm/svm/svm.h
    > +++ b/arch/x86/kvm/svm/svm.h
    > @@ -497,7 +497,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
    > #define AVIC_LOGICAL_ID_ENTRY_VALID_BIT 31
    > #define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1 << 31)
    >
    > -#define AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK (0xFFULL)
    > #define AVIC_PHYSICAL_ID_ENTRY_BACKING_PAGE_MASK (0xFFFFFFFFFFULL << 12)
    > #define AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK (1ULL << 62)
    > #define AVIC_PHYSICAL_ID_ENTRY_VALID_MASK (1ULL << 63)
    > --
    > 2.25.1
    >

    \
     
     \ /
      Last update: 2021-12-30 18:23    [W:4.123 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site