Re: [RFC 21/22] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Aug 01, 2016 at 05:57:50PM +0100, Julien Grall wrote:
> On 01/08/16 17:34, Mark Rutland wrote:
> >On Mon, Aug 01, 2016 at 04:40:50PM +0100, Julien Grall wrote:
> >>After I read again multiple time the ARM ARM (D4-1732 in ARM DDI
> >>0487A.i) and spoke with various ARM folks, changing the permission
> >>(i.e read, write, execute) does not require the break-before-make
> >>sequence.
> >
> >Regardless of whether Break-Before-Make (BBM) is required, TLB
> >invalidation is still required to ensure the new permissions have taken
> >effect after *any* modification to page tables, unless:
> >
> >* The prior entry was not permitted to be held in a TLB, and:
> >* No TLB held an entry for the address range.
> 
> Agreed, however we only need one TLBI instruction (assuming there is
> no superpage shattering) per-batch rather than one per-entry in this
> case.

I got Cc'd to a reply without the original patch context, so I'm not
sure what the case is here. I'm not exactly sure what you mean by
"per-batch".

Assuming that you've (only) changed the permissions (i.e. the AP bits
and XN bits) of a number of stage-2 leaf entries, you need either:

* A single TLBI VMALLE1IS

  Note that this removes *all* stage-2 or combined stage-1&2 entries
  from TLBs, and thus is stronger than necessary.

* Per entry, a TLBI IPAS2LE1IS

  e.g. 

    for_each_entry(x)
      modify_ap_bits(x);
    dsb(ishst);
    tlbi(ipas2le1is);
    dsb(ish);

> >>In the current implementation (i.e without this series), the TLB
> >>invalidation is deferred until the p2m is released. Until that time,
> >>a vCPU may still run with the previous permission and trap into the
> >>hypervisor the permission fault. However, as the permission as
> >>changed, p2m_memaccess_check may fail and we will inject an abort to
> >>the guest.
> >>
> >>The problem is very similar to [1]. This will still be true with
> >>this series applied if I relaxed the use of the break-before-make
> >>sequence. The two ways I can see to fix this are either try again
> >>the instruction (we would trap again if the permission was not
> >>correct) or keep the break-before-make.
> >
> >Why can you not have TLB invalidation without the full BBM sequence?
> 
> I agree that in general TLBI instruction does not require the full
> BBM sequence. If we ever need the TLB invalidation per entry, it
> will be better to keep BBM to keep the code streamlined.

If this is not performance-critical, this sounds fine.

This does unnecessarily penalise some cases, though.

e.g. a guest only performing reads if write permission is removed from
pages. 

> However, in this case I think it will be better to return to the
> guest and replay the instruction if a data/instruction abort has
> been received.

That sounds like what we do in Linux.

On a fault, if the page tables are such that the fault should not occur,
we assume we raced with concurrent modification, and return to the
faulting instruction. Eventually (once the TLB maintenance is
completed), the guest will make forward progress.

We have locking around page table manipulation, but only have to take
them in the (hopefully) unlikely case of a race on the affected memory
area.

Thanks,
Mark.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel