From: Sean Christopherson <seanjc@xxxxxxxxxx> Sent: Friday, February 10, 2023 3:47 PM
>
> On Fri, Feb 10, 2023, Michael Kelley (LINUX) wrote:
> > From: Sean Christopherson <seanjc@xxxxxxxxxx> Sent: Friday, February 10, 2023
> 12:58 PM
> > >
> > > On Fri, Feb 10, 2023, Sean Christopherson wrote:
> > > > On Fri, Feb 10, 2023, Dave Hansen wrote:
> > > > > On 2/10/23 11:36, Borislav Petkov wrote:
> > > > > >> One approach is to go with the individual device attributes for now.>> If the list
> > > does grow significantly, there will probably be patterns
> > > > > >> or groupings that we can't discern now. We could restructure into
> > > > > >> larger buckets at that point based on those patterns/groupings.
> > > > > > There's a reason the word "platform" is in cc_platform_has(). Initially
> > > > > > we wanted to distinguish attributes of the different platforms. So even
> > > > > > if y'all don't like CC_ATTR_PARAVISOR, that is what distinguishes this
> > > > > > platform and it *is* one platform.
> > > > > >
> > > > > > So call it CC_ATTR_SEV_VTOM as it uses that technology or whatever. But
> > > > > > call it like the platform, not to mean "I need this functionality".
> > > > >
> > > > > I can live with that. There's already a CC_ATTR_GUEST_SEV_SNP, so it
> > > > > would at least not be too much of a break from what we already have.
> > > >
> > > > I'm fine with CC_ATTR_SEV_VTOM, assuming the proposal is to have something
> like:
> > > >
> > > > static inline bool is_address_range_private(resource_size_t addr)
> > > > {
> > > > if (cc_platform_has(CC_ATTR_SEV_VTOM))
> > > > return is_address_below_vtom(addr);
> > > >
> > > > return false;
> > > > }
> > > >
> > > > i.e. not have SEV_VTOM mean "I/O APIC and vTPM are private". Though I don't
> see
> > > > the point in making it SEV vTOM specific or using a flag. Despite what any of us
> > > > think about TDX paravisors, it's completely doable within the confines of TDX to
> > > > have an emulated device reside in the private address space. E.g. why not
> > > > something like this?
> > > >
> > > > static inline bool is_address_range_private(resource_size_t addr)
> > > > {
> > > > return addr < cc_platform_private_end;
> > > > }
> > > >
> > > > where SEV fills in "cc_platform_private_end" when vTOM is enabled, and TDX does
> > > > the same. Or wrap cc_platform_private_end in a helper, etc.
> > >
> > > Gah, forgot that the intent with TDX is to enumerate devices in their legacy
> > > address spaces. So a TDX guest couldn't do this by default, but if/when Hyper-V
> > > or some other hypervisor moves I/O APIC, vTPM, etc... into the TCB, the common
> > > code would just work and only the hypervisor-specific paravirt code would need
> > > to change.
> > >
> > > Probably need a more specific name than is_address_range_private() though, e.g.
> > > is_mmio_address_range_private()?
> >
> > Maybe I'm not understanding what you are proposing, but in an SEV-SNP
> > VM using vTOM, devices like the IO-APIC and TPM live at their usual guest
> > physical addresses.
>
> Ah, so as the cover letter says, the intent really is to treat vTOM as an
> attribute bit. Sorry, I got confused by Boris's comment:
>
> : What happens if the next silly HV guest scheme comes along and they do
> : need more and different ones?
>
> Based on that comment, I assumed the proposal to use CC_ATTR_SEV_VTOM was
> intended
> to be a generic range-based thing, but it sounds like that's not the case.
>
> IMO, using CC_ATTR_SEV_VTOM to infer anything about the state of I/O APIC or vTPM
> is wrong. vTOM as a platform feature effectively says "physical address bit X
> controls private vs. shared" (ignoring weird usage of vTOM). vTOM does not mean
> I/O APIC and vTPM are private, that's very much a property of Hyper-V's current
> generation of vTOM-based VMs.
>
> Hardcoding this in the guest feels wrong. Ideally, we would have a way to enumerate
> that a device is private/trusted, e.g. through ACPI. I'm guessing we already
> missed the boat on that, so the next best thing is to do something like Michael
> originally proposed in this patch and shove the "which devices are private" logic
> into hypervisor-specific code, i.e. let Hyper-V figure out how to enumerate to its
> guests which devices are shared.
>
> I agree with Boris' comment that a one-off "other encrypted range" is a hack, but
> that's just an API problem. The kernel already has hypervisor specific hooks (and
> for SEV-ES even), why not expand that? That way figuring out which devices are
> private is wholly contained in Hyper-V code, at least until there's a generic
> solution for enumerating private devices, though that seems unlikely to happen
> and will be a happy problem to solve if it does come about.
Yes, this is definitely a cleaner way to implement what I was originally proposing.
I can make it work if there's agreement to take this approach.
Michael
>
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index a868b76cd3d4..08f65ed439d9 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2682,11 +2682,16 @@ static void io_apic_set_fixmap(enum fixed_addresses idx,
> phys_addr_t phys)
> {
> pgprot_t flags = FIXMAP_PAGE_NOCACHE;
>
> - /*
> - * Ensure fixmaps for IOAPIC MMIO respect memory encryption pgprot
> - * bits, just like normal ioremap():
> - */
> - flags = pgprot_decrypted(flags);
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> + /*
> + * Ensure fixmaps for IOAPIC MMIO respect memory encryption pgprot
> + * bits, just like normal ioremap():
> + */
> + if (x86_platform.hyper.is_private_mmio(phys))
> + flags = pgprot_encrypted(flags);
> + else
> + flags = pgprot_decrypted(flags);
> + }
>
> __set_fixmap(idx, phys, flags);
> }
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 6453fbaedb08..0baec766b921 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -116,6 +116,9 @@ static void __ioremap_check_other(resource_size_t addr, struct
> ioremap_desc *des
> if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> return;
>
> + if (x86_platform.hyper.is_private_mmio(addr))
> + desc->flags |= IORES_MAP_ENCRYPTED;
> +
> if (!IS_ENABLED(CONFIG_EFI))
> return;
>
