Re: [RFC 00/11] KVM, EFI, arm64: EFI Runtime Services Sandboxing

[Florent Revest]

On Fri, 2017-09-22 at 14:44 -0700, Ard Biesheuvel wrote:
> From the EFI side, there are some minor concerns on my part regarding
> the calling convention, and the fact that we can no longer invoke
> runtime services from a kernel running at EL1, but those all seem
> fixable. I will respond to the patches in question in greater detail
> at a later time.

Indeed, this RFC currently breaks EFI Runtime Services at EL1. This
would need to be fixed in a new patchset.

The patch 10/11 also underlines that the current argument passing
method does not respect alignment. The way arguments are currently
pushed and pulled makes it quite hard to fix the issue. Any suggestion
would be welcome.

> In the mean time, Christoffer has raised a number for valid concerns,
> and those need to be addressed first before it makes sense to talk
> about EFI specifics. I hope you will find more time to invest in
> this: I would really love to have this feature upstream.

Unfortunately, I'm no longer working at ARM and my other projects keep
me very busy. I would also love to invest more time in this patchset to
have it upstream but I'm really unsure when I will be able to find the
time for this.

Best,
    Florent
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 00/11] KVM, EFI, arm64: EFI Runtime Services Sandboxing

[Florent Revest]

On Thu, 2017-08-31 at 11:26 +0200, Christoffer Dall wrote:
> I wonder if this should be split into two series; one that sets up
> anything you may need from KVM, and another one that uses that for
> UEFI.
> 
> There's a lot KVM and UEFI intertwined logic and assumptions in patch
> 10, which makes this series a bit hard to read.

The way hypercalls are currently handled in handle_hvc required this
mixed patch. Would some kind of HVC subscription mechanism be suitable
to have in KVM? (e.g: a function allowing to register a callback on a
certain HVC function ID) This would allow the 10/11 patch to keep the
kvm code intact.

> I'd like some documentation (in the series and in
> Documentation/virtual/kvm) of how this works, and which hidden
> assumptions there are. For example, how do you ensure you never
> attempt to return to userspace?

I don't think my code ensured this. I'd need to give it a second look.

>  How many VCPUs do you support?

You can create as many VCPUs as you would in a "normal VM". Also, each
VCPU can be ran in a kthread.

>  Do you support any form of virtual interrupts? How about timers?

No support for virtual interrupts or timers indeed. The EFI Runtime
Services sandboxing wouldn't require that.

> Can a VM access physical devices?

The very idea of Runtime Services sandboxing requires Internal VMs to
have access to some of the physical devices.

>  How do you debug and trace something like this? Can the VM be
> monitored from userspace?

There is nothing ready for that.

> These feel like fundamental questions to me that needs addressing
> before I can competently review the code.
> 
> I think a slightly more concrete motivation and outlining the example
> of the broken UEFI on Seattle would help paving the way for these
> patches.

As far as I can remember, EFI Runtime Services on this platform have
already been reported to sometimes disable or enable interrupts. Maybe
someone at ARM has more details about the problem ?

Thanks a lot for your review,
    Florent
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs

[Florent Revest]

On Thu, 2017-08-31 at 11:23 +0200, Christoffer Dall wrote:
> > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> > index 2ea21da..1d2d3df 100644
> > --- a/virt/kvm/arm/mmu.c
> > +++ b/virt/kvm/arm/mmu.c
> > @@ -772,6 +772,11 @@ static void stage2_unmap_memslot(struct kvm
> > *kvm,
> > phys_addr_t size = PAGE_SIZE * memslot->npages;
> > hva_t reg_end = hva + size;
> > 
> > +   if (unlikely(!kvm->mm)) {
> I think you should consider using a predicate so that it's clear that
> this is for in-kernel VMs and not just some random situation where mm
> can be NULL.

Internal VMs should be the only usage when kvm->mm would be NULL.
However if you'd prefer it otherwise, I'll make sure this condition
will be made clearer.

> So it's unclear to me why we don't need any special casing in
> kvm_handle_guest_abort, related to MMIO exits etc.  You probably
> assume that we will never do emulation, but that should be described
> and addressed somewhere before I can critically review this patch.

This is indeed what I was assuming. This RFC does not allow MMIO with
internal VMs. I can not think of a usage when this would be useful. I'd
make sure this would be documented in an eventual later RFC.

> > +static int internal_vm_prep_mem(struct kvm *kvm,
> > +   const struct
> > kvm_userspace_memory_region *mem)
> > +{
> > +   phys_addr_t addr, end;
> > +   unsigned long pfn;
> > +   int ret;
> > +   struct kvm_mmu_memory_cache cache = { 0 };
> > +
> > +   end = mem->guest_phys_addr + mem->memory_size;
> > +   pfn = __phys_to_pfn(mem->guest_phys_addr);
> > +   addr = mem->guest_phys_addr;
> My main concern here is that we don't do any checks on this region
> and we could be mapping device memory here as well.  Are we intending
> that to be ok, and are we then relying on the guest to use proper
> memory attributes ?

Indeed, being able to map device memory is intended. It is needed for
Runtime Services sandboxing. It also relies on the guest being
correctly configured.

> > +
> > +   for (; addr < end; addr += PAGE_SIZE) {
> > +   pte_t pte = pfn_pte(pfn, PAGE_S2);
> > +
> > +   pte = kvm_s2pte_mkwrite(pte);
> > +
> > +   ret = mmu_topup_memory_cache(,
> > +KVM_MMU_CACHE_MIN_PAGE
> > S,
> > +KVM_NR_MEM_OBJS);
> You should be able to allocate all you need up front instead of doing
> it in sequences.

Ok.

> > 
> > +   if (ret) {
> > +   mmu_free_memory_cache();
> > +   return ret;
> > +   }
> > +   spin_lock(>mmu_lock);
> > +   ret = stage2_set_pte(kvm, , addr, , 0);
> > +   spin_unlock(>mmu_lock);
> Since you're likely to allocate some large contiguous chunks here,
> can you have a look at using section mappings?

Will do.

Thank you very much,
    Florent

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] tpm: rename event log provider files

[Jarkko Sakkinen]

On Wed, Sep 20, 2017 at 10:13:37AM +0200, Thiebaud Weksteen wrote:
> Rename the current TPM Event Log provider files (ACPI and OF)
> for clarity.
> 
> Signed-off-by: Thiebaud Weksteen 
> ---
>  drivers/char/tpm/Makefile| 4 ++--
>  drivers/char/tpm/{tpm_acpi.c => tpm_eventlog_acpi.c} | 0
>  drivers/char/tpm/{tpm_of.c => tpm_eventlog_of.c} | 0
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename drivers/char/tpm/{tpm_acpi.c => tpm_eventlog_acpi.c} (100%)
>  rename drivers/char/tpm/{tpm_of.c => tpm_eventlog_of.c} (100%)

Reviewed-by: Jarkko Sakkinen 

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html