RE: [PATCH v4 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.y

[Ram Pai]

On Thu, Dec 05, 2019 at 09:26:14AM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 05/12/2019 07:42, Ram Pai wrote:
> > On Wed, Dec 04, 2019 at 02:36:18PM +1100, David Gibson wrote:
> >> On Wed, Dec 04, 2019 at 12:08:09PM +1100, Alexey Kardashevskiy wrote:
> >>>
> >>>
> >>> On 04/12/2019 11:49, Ram Pai wrote:
> >>>> On Wed, Dec 04, 2019 at 11:04:04AM +1100, Alexey Kardashevskiy wrote:
> >>>>>
> >>>>>
> >>>>> On 04/12/2019 03:52, Ram Pai wrote:
> >>>>>> On Tue, Dec 03, 2019 at 03:24:37PM +1100, Alexey Kardashevskiy wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 03/12/2019 15:05, Ram Pai wrote:
> >>>>>>>> On Tue, Dec 03, 2019 at 01:15:04PM +1100, Alexey Kardashevskiy wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 03/12/2019 13:08, Ram Pai wrote:
> >>>>>>>>>> On Tue, Dec 03, 2019 at 11:56:43AM +1100, Alexey Kardashevskiy wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On 02/12/2019 17:45, Ram Pai wrote:
> >>>>>>>>>>>> H_PUT_TCE_INDIRECT hcall uses a page filled with TCE entries, as one of
> >>>>>>>>>>>> its parameters. One page is dedicated per cpu, for the lifetime of the
> >>>>>>>>>>>> kernel for this purpose. On secure VMs, contents of this page, when
> >>>>>>>>>>>> accessed by the hypervisor, retrieves encrypted TCE entries.  Hypervisor
> >>>>>>>>>>>> needs to know the unencrypted entries, to update the TCE table
> >>>>>>>>>>>> accordingly.  There is nothing secret or sensitive about these entries.
> >>>>>>>>>>>> Hence share the page with the hypervisor.
> >>>>>>>>>>>
> >>>>>>>>>>> This unsecures a page in the guest in a random place which creates an
> >>>>>>>>>>> additional attack surface which is hard to exploit indeed but
> >>>>>>>>>>> nevertheless it is there.
> >>>>>>>>>>> A safer option would be not to use the
> >>>>>>>>>>> hcall-multi-tce hyperrtas option (which translates FW_FEATURE_MULTITCE
> >>>>>>>>>>> in the guest).
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Hmm... How do we not use it?  AFAICT hcall-multi-tce option gets invoked
> >>>>>>>>>> automatically when IOMMU option is enabled.
> >>>>>>>>>
> >>>>>>>>> It is advertised by QEMU but the guest does not have to use it.
> >>>>>>>>
> >>>>>>>> Are you suggesting that even normal-guest, not use hcall-multi-tce?
> >>>>>>>> or just secure-guest?  
> >>>>>>>
> >>>>>>>
> >>>>>>> Just secure.
> >>>>>>
> >>>>>> hmm..  how are the TCE entries communicated to the hypervisor, if
> >>>>>> hcall-multi-tce is disabled?
> >>>>>
> >>>>> Via H_PUT_TCE which updates 1 entry at once (sets or clears).
> >>>>> hcall-multi-tce  enables H_PUT_TCE_INDIRECT (512 entries at once) and
> >>>>> H_STUFF_TCE (clearing, up to 4bln at once? many), these are simply an
> >>>>> optimization.
> >>>>
> >>>> Do you still think, secure-VM should use H_PUT_TCE and not
> >>>> H_PUT_TCE_INDIRECT?  And normal VM should use H_PUT_TCE_INDIRECT?
> >>>> Is there any advantage of special casing it for secure-VMs.
> >>>
> >>>
> >>> Reducing the amount of insecure memory at random location.
> >>
> >> The other approach we could use for that - which would still allow
> >> H_PUT_TCE_INDIRECT, would be to allocate the TCE buffer page from the
> >> same pool that we use for the bounce buffers.  I assume there must
> >> already be some sort of allocator for that?
> > 
> > The allocator for swiotlb is buried deep in the swiotlb code. It is 
> > not exposed to the outside-swiotlb world. Will have to do major surgery
> > to expose it.
> > 
> > I was thinking, maybe we share the page, finish the INDIRECT_TCE call,
> > and unshare the page.  This will address Alexey's concern of having
> > shared pages at random location, and will also give me my performance
> > optimization.  Alexey: ok?
> 
> 
> I really do not see the point. I really think we should to 1:1 mapping
> of swtiotlb buffers using the default 32bit window using H_PUT_TCE and
> this should be more than enough, I do not think the amount of code will
> be dramatically different compared to unsecuring and securing a page or
> using one of swtiotlb pages for this purpose. Thanks,

Ok. I will address your major concern -- "do not create new shared pages
at random location"  in my next version of the patch.  Using the 32bit
DMA window just to map the SWIOTLB buffers, will be some effort. Hope 
we can stage it that way.

RP