Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1195232 > unrolled thread

Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2015-07-29 16:20 +0200
    Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies Julien Grall <julien.grall@citrix.com> - 2015-07-29 16:30 +0200
      Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2015-07-29 17:00 +0200

#1195232 — Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies

On 07/29/2015 07:25 AM, Julien Grall wrote:
> Hi Boris,
>
> On 28/07/15 20:12, Boris Ostrovsky wrote:
>> On 07/28/2015 11:02 AM, Julien Grall wrote:
>>> Based on include/xen/mm.h [1], Linux is mistakenly using MFN when GFN
>>> is meant, I suspect this is because the first support for Xen was for
>>> PV. This brough some misimplementation of helpers on ARM and make the
>>> developper confused the expected behavior.
>>>
>>> For instance, with pfn_to_mfn, we expect to get an MFN based on the name.
>>> Although, if we look at the implementation on x86, it's returning a GFN.
>>>
>>> For clarity and avoid new confusion, replace any reference of mfn into
>>> gnf in any helpers used by PV drivers.
>>
>>
>>
>>
>>> @@ -730,7 +730,7 @@ static void xen_do_pin(unsigned level, unsigned
>>> long pfn)
>>>        struct mmuext_op op;
>>>
>>>        op.cmd = level;
>>> -    op.arg1.mfn = pfn_to_mfn(pfn);
>>> +    op.arg1.mfn = pfn_to_gfn(pfn);
>>
>>
>> This looks slightly odd. It is correct but given that purpose of this
>> series is to make things more clear perhaps we can add another union
>> member (gfn) to mmuext_op.arg1?
>>
>> (Of course, the hypervisor will continue referring to mfn which could
>> still be confusing)
>
> This operation is only used for PV guests, right?
>
> IHMO re-introducing pfn_to_mfn for PV-guests only (i.e with a BUG_ON to
> ensure no usage for auto-translated guest) would be the best solution.
> It would avoid to have different name than the hypersivor one in the
> hypercall interface. It will also make clear that virt_to_machine & co
> is only PV specific.
>
> I though doing this but I preferred to defer it to x86 expert as my
> knowledge for x86 Xen is very limited. I don't know where it's more
> suitable to use MFN or GFN. I guess this file (mmu.c) is mostly PV specific?
>
> Would something like below fine for you?
>
> static inline unsigned long pfn_to_mfn(unsigned long pfn)
> {
> 	unsigned long mfn;
>
> 	BUG_ON(xen_feature(XENFEAT_auto_translated_physmap));
>
> 	mfn = __pfn_to_mfn(pfn);
> 	if (mfn != INVALID_P2M_ENTRY)
> 		mfn &= ~(FOREIGN_FRAME_BIT | IDENTITY_FRAME_BIT);
>
> 	return mfn;
> }
>
> static inline unsigned long pfn_to_gfn(unsigned long pfn)
> {
> 	if (xen_feature(XENFEAT_autotranslated_physmap))
> 		return pfn;
> 	else
> 		return pfn_to_mfn(pfn);
> }


But you'd still say 'op.arg1.mfn = pfn_to_gfn(pfn);' in xen_do_pin() 
i.e. assign GFN to MFN, right? That's what I was referring to.

(In general, I am not sure a guest should ever use 'mfn' as it is purely 
a hypervisor construct. Including p2m, which I think should really be 
p2g as this is what we use to figure out what to stick into page tables)

-boris


>
> Similar splitting would be done for gfn_to_pfn and mfn_to_pfn.
>
> Regards,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1195243

On 29/07/15 15:14, Boris Ostrovsky wrote:
>> static inline unsigned long pfn_to_gfn(unsigned long pfn)
>> {
>>     if (xen_feature(XENFEAT_autotranslated_physmap))
>>         return pfn;
>>     else
>>         return pfn_to_mfn(pfn);
>> }
> 
> 
> But you'd still say 'op.arg1.mfn = pfn_to_gfn(pfn);' in xen_do_pin()
> i.e. assign GFN to MFN, right? That's what I was referring to.

Well no. I would use op.arg1.mfn = pfn_to_mfn(pfn) given that the code,
if I'm right, is only executed for PV.

mfn = pfn_to_gfn(...) was valid too because on PV is always an MFN. The
suggestion of pfn_to_mfn was just for more readability,

> (In general, I am not sure a guest should ever use 'mfn' as it is purely
> a hypervisor construct. Including p2m, which I think should really be
> p2g as this is what we use to figure out what to stick into page tables)

I think avoid to use mfn in the hypervisor interface is out-of-scope for
this series. If we ever want to modify the Xen API in Linux, we should
do in sync with Xen to avoid inconsistency on naming.

Anyway, the oddity of mfn = pfn_to_gfn(...) is mostly contained in the
x86 specific code. I don't mind to either add pfn_to_mfn and use it or
add a comment /* PV-specific so mfn == gfn */ for every use of mfn =
pfn_to_gfn(...).

Regards,

-- 
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1195268

On 07/29/2015 10:23 AM, Julien Grall wrote:
> On 29/07/15 15:14, Boris Ostrovsky wrote:
>>> static inline unsigned long pfn_to_gfn(unsigned long pfn)
>>> {
>>>      if (xen_feature(XENFEAT_autotranslated_physmap))
>>>          return pfn;
>>>      else
>>>          return pfn_to_mfn(pfn);
>>> }
>>
>> But you'd still say 'op.arg1.mfn = pfn_to_gfn(pfn);' in xen_do_pin()
>> i.e. assign GFN to MFN, right? That's what I was referring to.
> Well no. I would use op.arg1.mfn = pfn_to_mfn(pfn) given that the code,
> if I'm right, is only executed for PV.
>
> mfn = pfn_to_gfn(...) was valid too because on PV is always an MFN. The
> suggestion of pfn_to_mfn was just for more readability,

Right, and my comments were also not about correctness.


>
>> (In general, I am not sure a guest should ever use 'mfn' as it is purely
>> a hypervisor construct. Including p2m, which I think should really be
>> p2g as this is what we use to figure out what to stick into page tables)
> I think avoid to use mfn in the hypervisor interface is out-of-scope for
> this series. If we ever want to modify the Xen API in Linux, we should
> do in sync with Xen to avoid inconsistency on naming.
>
> Anyway, the oddity of mfn = pfn_to_gfn(...) is mostly contained in the
> x86 specific code. I don't mind to either add pfn_to_mfn and use it or
> add a comment /* PV-specific so mfn == gfn */ for every use of mfn =
> pfn_to_gfn(...).

I think the former is better (even thought it adds a test)

-boris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web