Hi Alex,

On 2021-02-12 17:12, Alexandru Elisei wrote:

Hi Marc,

I've been trying to get my head around what the architecture says about 
CMOs, so
please bare with me if I misunderstood some things.

No worries. I've had this patch for a few weeks now, and can't
make up my mind about it. It does address an actual issue though,
so I couldn't just discard it... ;-)

On 2/11/21 2:27 PM, Marc Zyngier wrote:

quoted

It appears that when a guest traps into KVM because it is
performing a CMO on a Read Only memslot, our handling of
this operation is "slightly suboptimal", as we treat it as
an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal
with such an exception being slim, it would be better to
handle it in the kernel.

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
  (1) if the CMO is DC IVAC, the architecture says this should result
      in a permission fault
  (2) if the CMO is DC CIVAC, it should work similarly to (a)

When you say it should work similarly to (a), you mean it should be 
handled as a
normal memory access, without the "CMO hits writeable memory" part, 
right?

What I mean is that the cache invalidation should take place,
preferably without involving KVM at all (other than populating
S2 if required).

quoted

We already perform (a) and (b) correctly, but (c) is a total mess.
Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as
a *read*, even when they are on a RW memslot. This allows us to
further triage things:

If they come back with a permission fault, that is because this is
a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO
mapping, and once for the permission fault. I think we can live with
that.

I'm trying to make sure I understand what the problem is.

gfn_to_pfn_prot() returnsKVM_HVA_ERR_RO_BAD if the write is to a RO 
memslot.
KVM_HVA_ERR_RO_BAD is PAGE_OFFSET + PAGE_SIZE, which means that
is_error_noslot_pfn() return true. In that case we exit to userspace
with -EFAULT
for DC IVAC and DC CIVAC. But what we should be doing is this:

- For DC IVAC, inject a dabt with ISS = 0x10, meaning an external abort 
(that's
what kvm_inject_dabt_does()).

- For DC CIVAC, exit to userspace with -EFAULT.

Did I get that right?

Not quite. What I *think* we should do is:

- DC CIVAC should just work, without going to userspace. I can't imagine
   a reason why we'd involve userspace for this, and we currently don't
   really have a good way to describe this to userspace.

- DC IVAC is more nuanced: we could either inject an exception (which
   is what this patch does), or perform the CMO ourselves as a DC CIVAC
   (consistent with the IVA->CIVA upgrade caused by having a S2 
translation).
   This second approach is comparable to what we do when the guest
   issues a CMO on an emulated MMIO address (we don't inject a fault).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel