coconut-svsm / svsm

COCONUT-SVSM
MIT License
113 stars 39 forks source link

platform: extend the way pages are identified for validation/invalidation #461

Closed msft-jlange closed 2 days ago

msft-jlange commented 2 weeks ago

SNP validates pages by virtual address while TDX validates pages by physical address. This PR makes it possible for code to request validation changes based on either. This ensures that virtual mappings are not created unnecessarily on platforms that do not require them, and ensures that it can be possible to correctly obtain the physical address for validation in those cases where the caller has only a virtual address.

msft-jlange commented 2 weeks ago

@peterfang This should resolve the challenges you had in implementing validation for the TDP platform.

peterfang commented 1 week ago

@peterfang This should resolve the challenges you had in implementing validation for the TDP platform.

Thanks! Will update my stage2 PR after this is merged.

joergroedel commented 1 week ago

These changes break boot for me, I need to investigate before this can be merged.

roy-hopkins commented 3 days ago

This breaks the boot for me too. I'll help investigate.

Freax13 commented 3 days ago

I've started on this as well. AFAICT the self-map isn't set up properly for some (or all?) page tables, so the lookups fail. AFAICT the self-map sometimes points to random garbage, I'm not yet sure why.

msft-jlange commented 2 days ago

I've fixed the PR. There were two problems: neither the self-map PML4E nor the level 1/2/3 page table hierarchy logic would set the C-bit in any of those entries. With a self-map, every intermediate entry can be treated like a PTE, so all of them need to set the C-bit. The Hyper-V and TDX environments both use vTOM (or a vTOM equivalent) which is why no problem was observed in either environment. I've updated the PR to set the C-bit correctly in non-vTOM environments and everything appears to work now.

BTW, the reason the failures appeared to be so random is that with the C-bit clear, accesses through the self-map would result in reading ciphertext, which is random.

roy-hopkins commented 2 days ago

I've fixed the PR.

I've tested this new version and it works in my environment.