Re: [Xen-devel] [PATCH v4 01/15] re-work commit 3e06b989 "IOMMU: make page table population preemptible"...

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.08.18 at 10:49, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 02 August 2018 09:19
>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
>> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
>> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
>> <wei.liu2@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
>> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
>> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
>> Subject: RE: [PATCH v4 01/15] re-work commit 3e06b989 "IOMMU: make
>> page table population preemptible"...
>> 
>> >>> On 02.08.18 at 10:04, <Paul.Durrant@xxxxxxxxxx> wrote:
>> >> From: Paul Durrant
>> >> Sent: 02 August 2018 09:03
>> >> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >> > Sent: 02 August 2018 08:20
>> >> > >>> On 01.08.18 at 15:40, <paul.durrant@xxxxxxxxxx> wrote:
>> >> > > ...to simplify the implementation and turn need_iommu back into a
>> >> > boolean.
>> >> > >
>> >> > > As noted in [1] the tri-state nature of need_iommu after commit
>> >> 3e06b989
>> >> > is
>> >> > > confusing, as is the implementation of pre-emption using relmem_list.
>> >> > >
>> >> > > This patch instead uses a simple count of pages already populated
>> stored
>> >> in
>> >> > > the x86 variant of struct arch_iommu and skips over that number of
>> >> pages
>> >> > > if arch_iommu_populate_page_table() is re-invoked after pre-
>> emption.
>> >> >
>> >> > Well, yes, I would have used that model in said commit if it was 
>> >> > reliable,
>> >> > but it isn't: What if the list of pages changed between two (re-
>> )invocations?
>> >>
>> >> Is that really going to happen? This is the result of a domctl, which is a
>> tools-
>> >> only hypercall right?
>> >
>> > Oh, I see what you mean... the guest could do something like a
>> > decrease_reservation... I was overlooking that setting up the iommu is
>> > happening while the guest is live. Would it be reasonable to
>> domain_pause()
>> > for safety then?
>> 
>> I'm hesitant to see domains (or vcpus) paused other than when absolutely
>> necessary. If everyone else thinks this is a good idea here, I think I won't
>> object, but please don't forget that any pausing for perhaps an extended
>> period of time may cause the guest to misbehave subsequently.
>> 
> 
> Yes, true, but I also wonder how safe it is to empty the page_list of a 
> running guest. I guess it may be the best way but I think having a dedicate 
> page_list in the iommu struct to host pages that have already been mapped and 
> then transfer them back at the end would be cleaner and should allow 
> need_iommu to stay boolean.

The issue is with the case of the guest trying to free a page - see
arch_free_heap_page(): The way page lists work when link fields
are 32-bit PDXes, all lists a page may possibly be on need to be
inspected, in case the page is at the head of that list. Introducing a
3rd list to take into consideration seems undesirable to me (and
again I did consider that option back when writing things the way
they are now).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel