Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
Hi Chris, On 28/07/15 20:39, Chris (Christopher) Brand 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 Typo : brought Perhaps resulted in would be better ? misimplementation of helpers on ARM and make the developper confused the expected behavior. Typo: developer. I'd also suggest ...and confused developers about the [...] For clarity and avoid new confusion, replace any reference of mfn into gnf in any helpers used by PV drivers. Typo : gfn I'd suggest ...replace any reference to mfn with gfn... [...] Thanks for telling me the typoes. I will fix it in the next version of this series. Regards, -- Julien Grall -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
On Wed, Jul 29, 2015 at 12:35:54PM +0100, Julien Grall wrote:
Hi Wei,
On 29/07/15 11:13, Wei Liu wrote:
On Tue, Jul 28, 2015 at 04:02:45PM +0100, Julien Grall wrote:
[...]
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 7d50711..3b7b7c3 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -314,7 +314,7 @@ static void xenvif_gop_frag_copy(struct xenvif_queue
*queue, struct sk_buff *skb
} else {
copy_gop-source.domid = DOMID_SELF;
copy_gop-source.u.gmfn =
- virt_to_mfn(page_address(page));
+ virt_to_gfn(page_address(page));
}
copy_gop-source.offset = offset;
@@ -1284,7 +1284,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue
*queue,
queue-tx_copy_ops[*copy_ops].source.offset = txreq.offset;
queue-tx_copy_ops[*copy_ops].dest.u.gmfn =
- virt_to_mfn(skb-data);
+ virt_to_gfn(skb-data);
queue-tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
queue-tx_copy_ops[*copy_ops].dest.offset =
offset_in_page(skb-data);
Reviewed-by: Wei Liu wei.l...@citrix.com
One possible improvement is to change gmfn in copy_gop to gfn as well.
But that's outside of netback code.
The structure gnttab_copy is part of the hypervisor interface. Is it
fine to differ on the naming between Xen and Linux?
Or maybe we could do the change in the public headers in Xen repo too.
Is it fine to do field renaming in public headers?
Oh well. Never mind then. I mistook that structure as internal to Linux.
Wei.
Regards,
--
Julien Grall
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
Hi Wei,
On 29/07/15 11:13, Wei Liu wrote:
On Tue, Jul 28, 2015 at 04:02:45PM +0100, Julien Grall wrote:
[...]
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 7d50711..3b7b7c3 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -314,7 +314,7 @@ static void xenvif_gop_frag_copy(struct xenvif_queue
*queue, struct sk_buff *skb
} else {
copy_gop-source.domid = DOMID_SELF;
copy_gop-source.u.gmfn =
-virt_to_mfn(page_address(page));
+virt_to_gfn(page_address(page));
}
copy_gop-source.offset = offset;
@@ -1284,7 +1284,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue
*queue,
queue-tx_copy_ops[*copy_ops].source.offset = txreq.offset;
queue-tx_copy_ops[*copy_ops].dest.u.gmfn =
-virt_to_mfn(skb-data);
+virt_to_gfn(skb-data);
queue-tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
queue-tx_copy_ops[*copy_ops].dest.offset =
offset_in_page(skb-data);
Reviewed-by: Wei Liu wei.l...@citrix.com
One possible improvement is to change gmfn in copy_gop to gfn as well.
But that's outside of netback code.
The structure gnttab_copy is part of the hypervisor interface. Is it
fine to differ on the naming between Xen and Linux?
Or maybe we could do the change in the public headers in Xen repo too.
Is it fine to do field renaming in public headers?
Regards,
--
Julien Grall
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
On 29/07/15 12:35, Julien Grall wrote:
Hi Wei,
On 29/07/15 11:13, Wei Liu wrote:
On Tue, Jul 28, 2015 at 04:02:45PM +0100, Julien Grall wrote:
[...]
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 7d50711..3b7b7c3 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -314,7 +314,7 @@ static void xenvif_gop_frag_copy(struct xenvif_queue
*queue, struct sk_buff *skb
} else {
copy_gop-source.domid = DOMID_SELF;
copy_gop-source.u.gmfn =
- virt_to_mfn(page_address(page));
+ virt_to_gfn(page_address(page));
}
copy_gop-source.offset = offset;
@@ -1284,7 +1284,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue
*queue,
queue-tx_copy_ops[*copy_ops].source.offset = txreq.offset;
queue-tx_copy_ops[*copy_ops].dest.u.gmfn =
- virt_to_mfn(skb-data);
+ virt_to_gfn(skb-data);
queue-tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
queue-tx_copy_ops[*copy_ops].dest.offset =
offset_in_page(skb-data);
Reviewed-by: Wei Liu wei.l...@citrix.com
One possible improvement is to change gmfn in copy_gop to gfn as well.
But that's outside of netback code.
The structure gnttab_copy is part of the hypervisor interface. Is it
fine to differ on the naming between Xen and Linux?
Or maybe we could do the change in the public headers in Xen repo too.
Is it fine to do field renaming in public headers?
I think this series should not alter than Xen API.
David
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
On 28/07/15 18:16, David Vrabel wrote: On 28/07/15 16:02, 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 the benefit of other subsystem maintainers, this is a purely mechanical change in Xen-specific terminology. It doesn't need reviews or acks from non-Xen people (IMO). 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. Take also the opportunity to simplify simple construction such as pfn_to_mfn(page_to_pfn(page)) into page_to_gfn. More complex clean up will come in follow-up patches. I think it may be possible to do further clean up in the x86 code to ensure that helpers returning machine address (such as virt_address) is not used by no auto-translated guests. I will let x86 xen expert doing it. Reviewed-by: David Vrabel david.vra...@citrix.com It looks a bit odd to use GFN in some of the PV code where the hypervisor API uses MFN but overall I think using the correct terminology where possible is best. But I'd like to have Boris's or Konrad's opinion on this. I was thinking to introduce mfn_to_pfn co which would be used only for PV-guest (a BUG_ON would be here to ensure it) and hypercall related. I didn't do it as I haven't much knowledge on x86 Xen and was able to decide where I have to use pfn_to_mfn. Regards, -- Julien Grall -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
On 28/07/15 16:02, 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 the benefit of other subsystem maintainers, this is a purely mechanical change in Xen-specific terminology. It doesn't need reviews or acks from non-Xen people (IMO). 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. Take also the opportunity to simplify simple construction such as pfn_to_mfn(page_to_pfn(page)) into page_to_gfn. More complex clean up will come in follow-up patches. I think it may be possible to do further clean up in the x86 code to ensure that helpers returning machine address (such as virt_address) is not used by no auto-translated guests. I will let x86 xen expert doing it. Reviewed-by: David Vrabel david.vra...@citrix.com It looks a bit odd to use GFN in some of the PV code where the hypervisor API uses MFN but overall I think using the correct terminology where possible is best. But I'd like to have Boris's or Konrad's opinion on this. David -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
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
Typo : brought
Perhaps resulted in would be better ?
misimplementation of helpers on ARM and make the developper confused the
expected behavior.
Typo: developer.
I'd also suggest ...and confused developers about the
[...]
For clarity and avoid new confusion, replace any reference of mfn into gnf in
any helpers used by PV drivers.
Typo : gfn
I'd suggest ...replace any reference to mfn with gfn...
[...]
Chris
N�r��yb�X��ǧv�^�){.n�+{��zn�)w* jg��� �ݢj/���z�ޖ��2�ޙ�)ߡ�a���� �G���h��j:+v���w��٥
