MADT found but not valid?

[mrjbom]

Hi.
I have successfully created an instance of AcpiTables and found the MADT table. But it is not passing validation.

// Get RSDP address
let rsdp_phys_addr = boot_info
        .rsdp_addr
        .into_option()
        .expect("Bootloader could not find RSDP");

// Validate RSDP
    let rsdp =
        VirtAddr::new(rsdp_phys_addr + PHYSICAL_MEMORY_MAPPING_OFFSET).as_ptr::<acpi::rsdp::Rsdp>();
    unsafe {
        (*rsdp).validate().expect("Invalid RSDP!");
    }

// Create ACPI tables
    let acpi_tables = unsafe {
        AcpiTables::from_rsdp(BaseAcpiHandler, rsdp_phys_addr as usize)
            .expect("Failed to get ACPI tables")
    };

// Find MADT
let madt = *acpi_tables.find_table::<acpi::madt::Madt>().expect("Failed to find MADT");

madt.validate().expect("Failed to validate MADT");

I get this error: "Failed to validate MADT: SdtInvalidChecksum("APIC")"

I suspect that the wrong data is being read, an incorrect implementation of AcpiHeader may be to blame.
My physical address conversion is simple, I just add an offset to the physical address and get a virtual one.
This code is for x86-64:

impl acpi::AcpiHandler for BaseAcpiHandler {
    unsafe fn map_physical_region<T>(
        &self,
        physical_address: usize,
        _size: usize,
    ) -> PhysicalMapping<Self, T> {
        // We just need to return the virtual address from Complete Physical Memory Mapping region

        // Align physical_address to page start
        let physical_region_start = x86_64::align_down(physical_address as u64, PAGE_SIZE as u64);
        let physical_region_size = {
            let size = physical_address as u64 - physical_region_start;
            if size == 0 {
                PAGE_SIZE
            } else {
                x86_64::align_up(size, PAGE_SIZE as u64) as usize
            }
        };
        debug_assert_eq!(physical_region_start as usize % PAGE_SIZE, 0);
        debug_assert!(physical_region_size >= PAGE_SIZE);

        let virtual_address =
            VirtAddr::new(physical_address as u64 + PHYSICAL_MEMORY_MAPPING_OFFSET);

        PhysicalMapping::new(
            physical_address,
            NonNull::new(virtual_address.as_mut_ptr::<T>()).unwrap(),
            size_of::<T>(),
            physical_region_size,
            self.clone(),
        )
    }

    fn unmap_physical_region<T>(_region: &PhysicalMapping<Self, T>) {
        // There is no need to do anything
    }
}

Did I understand the PhysicalMapping::new() parameters correctly?

[mrjbom]

I'm currently investigating the code and so far I've found an extremely strange thing:

let madt = *acpi_tables_mutex_guard.get().unwrap().find_table::<Madt>().expect("Failed to find MADT");
let madt_ref = unsafe {
    acpi_tables_mutex_guard.get().unwrap().find_table::<Madt>().expect("Failed to find MADT").virtual_start().as_ref()
};

let bytes_from_var = core::ptr::slice_from_raw_parts::<u8>(&raw const madt as *mut u8, madt.header.length as usize);
let bytes_from_ref = core::ptr::slice_from_raw_parts::<u8>(madt_ref as *const Madt as *mut u8, madt.header.length as usize);

// False!
log::debug!("manual validate var: {}", (*bytes_from_var).iter().fold(0u8, |sum, &byte| sum.wrapping_add(byte)) == 0);

// True!
log::debug!("manual validate ref: {}", (*bytes_from_ref).iter().fold(0u8, |sum, &byte| sum.wrapping_add(byte)) == 0);

[mrjbom]

Okay, there is definitely a problem with cloning a variable. The cloned variable does not match what is in the reference it is cloned from.

The corruption appears to be at the byte level.
If I print out the values of both variables, they are exactly the same, but if I convert them to a slice of bytes, then these bytes are different.

unsafe {
    let rsdp_phys_addr = boot_info.rsdp_addr.into_option().unwrap() as usize;
    let acpi_tables = ::acpi::AcpiTables::from_rsdp(crate::acpi::BaseAcpiHandler, rsdp_phys_addr).unwrap();
    let madt_mapping = acpi_tables.find_table::<Madt>().unwrap();

    let madt_ref = madt_mapping.virtual_start().as_ref();
    let madt_cloned_from_ref = madt_ref.clone();

    let bytes_madt_ref = core::ptr::slice_from_raw_parts::<u8>(madt_ref as *const Madt as _, madt_ref.header.length as usize);
    let bytes_madt_cloned_from_ref = core::ptr::slice_from_raw_parts::<u8>(&raw const madt_cloned_from_ref as _, madt_cloned_from_ref.header.length as usize);

    // FAILS!
    assert_eq!(*bytes_madt_ref, *bytes_madt_cloned_from_ref);
}

[mrjbom]

I realised the mistake.

Because of thoughtlessly placed #[derive(Debug, Clone, Copy)] the structure is copied, but MADT Entries are not. So this copy is no longer valid and MADT Entries can no longer be accessed. Cool. Need to remove Clone for this structure, I guess there are other places where this problem exists.