Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Anthony Liguori <anthony@codemonkey.ws>]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Tue, 2013-11-05 at 12:53 -0800, Anthony Liguori wrote:
>> >> Yes, you are completely right, then I have to figure out why blkback
>> >> works fine with this patch applied (or at least it seems to work fine).
>> >
>> > blkback also works for me when testing a similar patch. I'm still
>> > confused. One thing with your proposed patch: I'm not sure that you're
>> > putting back the correct mfn.
>
> As long as it is unique and owned by the local domain I guess it doesn't
> matter which mfn goes back? It's going to get given back to the generic
> allocator so the content is irrelevant? (all speculation without looking
> at blkback)


Hrm, so m2p_add_override already does set_phys_to_machine(pfn,
FOREIGN_FRAME(mfn)) so this patch doesn't add anything that isn't
already there.  It's just removing the additional to the m2p override
table.

So what happens when the MFN isn't in the m2p override table?  It's
treated as a foreign PFN and m2p lookups fail.  But why is this a
problem for blkback?

>> It's perfectly fine to store a foreign pfn in the m2p table.
>
> No, it's not. The m2p is host global and maps mfns to the pfns in the
> owning domain. A domain which grant maps a foreign mfn into its address
> space doesn't get to update the m2p for that mfn. Perhaps you were
> thinking of the p2m?

Yes, I typoed that bit.

> Depending on how an OS mapping the foreign MFN does things it may well
> be accurate to say that having a foreign pfn in the m2p table is
> harmless, in as much as it will never try and use the m2p for foreign
> pages for anything real, but it depends on the implementation of the
> particular OS.

Note that this patch isn't doing anything to the m2p table.

>>   The m2p
>> override table is used by the grant device to allow a reverse lookup of
>> the real mfn to a pfn even if it's foreign.
>
> Classic Xen used an array of struct page * overrides in the struct vma
> for this sort of use case (e.g. in blktap). That obviously cannot fly
> for pvops...

I don't think the usage from blkback precludes using the gntdev either.
It just so happens that the m2p_add_override does extra work but I don't
think it actually benefits blkback in any way.

Am I missing something?

Regards,

Anthony Liguori

>
> Ian.