Re: [PATCH 0/2] kvm: IOMMU read-only mapping support

Xiao Guangrong
2013-01-30T04:06:42Z

On 01/29/2013 03:48 PM, Gleb Natapov wrote:
> On Tue, Jan 29, 2013 at 03:37:23PM +0800, Xiao Guangrong wrote:
>> On 01/29/2013 02:50 PM, Gleb Natapov wrote:
>>> On Tue, Jan 29, 2013 at 11:06:43AM +0800, Xiao Guangrong wrote:
>>>> On 01/28/2013 06:59 PM, Gleb Natapov wrote:
>>>>> On Fri, Jan 25, 2013 at 11:28:40AM +0800, Xiao Guangrong wrote:
>>>>>> On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote:
>>>>>>> On Thu, 24 Jan 2013 15:03:57 -0700
>>>>>>> Alex Williamson <alex.williamson@redhat.com> wrote:
>>>>>>>
>>>>>>>> A couple patches to make KVM IOMMU support honor read-only mappings.
>>>>>>>> This causes an un-map, re-map when the read-only flag changes and
>>>>>>>> makes use of it when setting IOMMU attributes. Thanks,
>>>>>>>
>>>>>>> Looks good to me.
>>>>>>>
>>>>>>> I think I can naturally update my patch after this gets merged.
>>>>>>>
>>>>>>
>>>>>> Please wait.
>>>>>>
>>>>>> The commit c972f3b1 changed the write-protect behaviour - it does
>>>>>> wirte-protection only when dirty flag is set.
>>>>>> [ I did not see this commit when we discussed the problem before. ]
>>>>>>
>>>>>> Further more, i notice that write-protect is not enough, when do sync
>>>>>> shadow page:
>>>>>>
>>>>>> FNAME(sync_page):
>>>>>>
>>>>>> host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE;
>>>>>>
>>>>>> set_spte(vcpu, &sp->spt[i], pte_access,
>>>>>> PT_PAGE_TABLE_LEVEL, gfn,
>>>>>> spte_to_pfn(sp->spt[i]), true, false,
>>>>>> host_writable);
>>>>>>
>>>>>> It sets spte based on the old value that means the readonly flag check
>>>>>> is missed. We need to call kvm_arch_flush_shadow_all under this case.
>>>>> Why not just disallow changing memory region KVM_MEM_READONLY flag
>>>>> without deleting the region?
>>>>
>>>> It will introduce some restriction when VM-sharing-mem is being implemented,
>>>> but we need to do some optimization for it, at least, properly write-protect
>>>> readonly pages (fix sync_page()) instead of zap_all_page.
>>>>
>>> What is VM-sharing-mem?
>>
>> Sharing memory between different guests.
>>
> That much I can figure out for the name itself. My question is how this
> sharing will work? Why KVM_MEM_READONLY is needed for it? Why ability to
> change KVM_MEM_READONLY flag without destroying memory region will be
> important. What's wrong with nahanni, shared memory device we have today?

I'm not clear now or maybe my memory is wrong, i need to find the origin
discussion...

>
>>>
>>>> So, i guess we can do the simple fix first.
>>>>
>>> By simple fix you mean calling kvm_arch_flush_shadow_all() on READONLY
>>> flag change?
>>
>> Simply disallow READONLY flag changing.
> Ok, can somebody craft a patch?

Takuya, will you? ;)