* [PATCH] x86/mm: drop further relics of translated PV domains
@ 2017-06-08 15:30 Jan Beulich
2017-06-09 17:38 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2017-06-08 15:30 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 5923 bytes --]
For PV domains paging_mode_{refcounts,translate}() are always false as
of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
== PG_refcounts") and 92942fd3d4 ("x86/mm: drop
guest_{map,get_eff}_l1e() hooks").
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1591,7 +1591,7 @@ void init_guest_l4_table(l4_pgentry_t l4
l4e_from_pfn(domain_page_map_to_mfn(l4tab), __PAGE_HYPERVISOR_RW);
l4tab[l4_table_offset(PERDOMAIN_VIRT_START)] =
l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
- if ( zap_ro_mpt || is_pv_32bit_domain(d) || paging_mode_refcounts(d) )
+ if ( zap_ro_mpt || is_pv_32bit_domain(d) )
l4tab[l4_table_offset(RO_MPT_VIRT_START)] = l4e_empty();
}
@@ -1902,12 +1902,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
if ( unlikely(__copy_from_user(&ol1e, pl1e, sizeof(ol1e)) != 0) )
return -EFAULT;
- if ( unlikely(paging_mode_refcounts(pt_dom)) )
- {
- if ( UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, preserve_ad) )
- return 0;
- return -EBUSY;
- }
+ ASSERT(!paging_mode_refcounts(pt_dom));
if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
{
@@ -2359,8 +2354,7 @@ int free_page_type(struct page_info *pag
/* A page table is dirtied when its type count becomes zero. */
paging_mark_dirty(owner, _mfn(page_to_mfn(page)));
- if ( shadow_mode_refcounts(owner) )
- return 0;
+ ASSERT(!shadow_mode_refcounts(owner));
gmfn = mfn_to_gmfn(owner, page_to_mfn(page));
ASSERT(VALID_M2P(gmfn));
@@ -2960,14 +2954,11 @@ int new_guest_cr3(unsigned long mfn)
unsigned long gt_mfn = pagetable_get_pfn(curr->arch.guest_table);
l4_pgentry_t *pl4e = map_domain_page(_mfn(gt_mfn));
- rc = paging_mode_refcounts(d)
- ? -EINVAL /* Old code was broken, but what should it be? */
- : mod_l4_entry(
- pl4e,
- l4e_from_pfn(
- mfn,
- (_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_ACCESSED)),
- gt_mfn, 0, curr);
+ rc = mod_l4_entry(pl4e,
+ l4e_from_pfn(mfn,
+ (_PAGE_PRESENT | _PAGE_RW |
+ _PAGE_USER | _PAGE_ACCESSED)),
+ gt_mfn, 0, curr);
unmap_domain_page(pl4e);
switch ( rc )
{
@@ -3069,13 +3060,6 @@ static struct domain *get_pg_owner(domid
goto out;
}
- if ( !is_hvm_domain(curr) && unlikely(paging_mode_translate(curr)) )
- {
- gdprintk(XENLOG_WARNING,
- "Cannot mix foreign mappings with translated domains\n");
- goto out;
- }
-
switch ( domid )
{
case DOMID_IO:
@@ -3384,11 +3368,9 @@ long do_mmuext_op(
if ( op.arg1.mfn != 0 )
{
- if ( paging_mode_refcounts(d) )
- rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
- else
- rc = get_page_and_type_from_pagenr(
- op.arg1.mfn, PGT_root_page_table, d, 0, 1);
+ rc = get_page_and_type_from_pagenr(op.arg1.mfn,
+ PGT_root_page_table,
+ d, 0, 1);
if ( unlikely(rc) )
{
@@ -3400,7 +3382,7 @@ long do_mmuext_op(
rc, op.arg1.mfn);
break;
}
- if ( VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
+ if ( VM_ASSIST(d, m2p_strict) )
zap_ro_mpt(op.arg1.mfn);
}
@@ -3410,21 +3392,18 @@ long do_mmuext_op(
{
page = mfn_to_page(old_mfn);
- if ( paging_mode_refcounts(d) )
- put_page(page);
- else
- switch ( rc = put_page_and_type_preemptible(page) )
- {
- case -EINTR:
- rc = -ERESTART;
- /* fallthrough */
- case -ERESTART:
- curr->arch.old_guest_table = page;
- break;
- default:
- BUG_ON(rc);
- break;
- }
+ switch ( rc = put_page_and_type_preemptible(page) )
+ {
+ case -EINTR:
+ rc = -ERESTART;
+ /* fallthrough */
+ case -ERESTART:
+ curr->arch.old_guest_table = page;
+ break;
+ default:
+ BUG_ON(rc);
+ break;
+ }
}
break;
@@ -4035,8 +4014,7 @@ static int create_grant_pte_mapping(
page_unlock(page);
- if ( !paging_mode_refcounts(d) )
- put_page_from_l1e(ol1e, d);
+ put_page_from_l1e(ol1e, d);
failed:
unmap_domain_page(va);
@@ -4162,7 +4140,7 @@ static int create_grant_va_mapping(
put_page(l1pg);
guest_unmap_l1e(pl1e);
- if ( okay && !paging_mode_refcounts(d) )
+ if ( okay )
put_page_from_l1e(ol1e, d);
return okay ? GNTST_okay : GNTST_general_error;
@@ -4387,7 +4365,7 @@ int replace_grant_host_mapping(
guest_unmap_l1e(pl1e);
rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
- if ( rc && !paging_mode_refcounts(curr->domain) )
+ if ( rc )
put_page_from_l1e(ol1e, curr->domain);
return rc;
[-- Attachment #2: x86-drop-paging-mode-checks.patch --]
[-- Type: text/plain, Size: 5975 bytes --]
x86/mm: drop further relics of translated PV domains
For PV domains paging_mode_{refcounts,translate}() are always false as
of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
== PG_refcounts") and 92942fd3d4 ("x86/mm: drop
guest_{map,get_eff}_l1e() hooks").
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1591,7 +1591,7 @@ void init_guest_l4_table(l4_pgentry_t l4
l4e_from_pfn(domain_page_map_to_mfn(l4tab), __PAGE_HYPERVISOR_RW);
l4tab[l4_table_offset(PERDOMAIN_VIRT_START)] =
l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
- if ( zap_ro_mpt || is_pv_32bit_domain(d) || paging_mode_refcounts(d) )
+ if ( zap_ro_mpt || is_pv_32bit_domain(d) )
l4tab[l4_table_offset(RO_MPT_VIRT_START)] = l4e_empty();
}
@@ -1902,12 +1902,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
if ( unlikely(__copy_from_user(&ol1e, pl1e, sizeof(ol1e)) != 0) )
return -EFAULT;
- if ( unlikely(paging_mode_refcounts(pt_dom)) )
- {
- if ( UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, preserve_ad) )
- return 0;
- return -EBUSY;
- }
+ ASSERT(!paging_mode_refcounts(pt_dom));
if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
{
@@ -2359,8 +2354,7 @@ int free_page_type(struct page_info *pag
/* A page table is dirtied when its type count becomes zero. */
paging_mark_dirty(owner, _mfn(page_to_mfn(page)));
- if ( shadow_mode_refcounts(owner) )
- return 0;
+ ASSERT(!shadow_mode_refcounts(owner));
gmfn = mfn_to_gmfn(owner, page_to_mfn(page));
ASSERT(VALID_M2P(gmfn));
@@ -2960,14 +2954,11 @@ int new_guest_cr3(unsigned long mfn)
unsigned long gt_mfn = pagetable_get_pfn(curr->arch.guest_table);
l4_pgentry_t *pl4e = map_domain_page(_mfn(gt_mfn));
- rc = paging_mode_refcounts(d)
- ? -EINVAL /* Old code was broken, but what should it be? */
- : mod_l4_entry(
- pl4e,
- l4e_from_pfn(
- mfn,
- (_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_ACCESSED)),
- gt_mfn, 0, curr);
+ rc = mod_l4_entry(pl4e,
+ l4e_from_pfn(mfn,
+ (_PAGE_PRESENT | _PAGE_RW |
+ _PAGE_USER | _PAGE_ACCESSED)),
+ gt_mfn, 0, curr);
unmap_domain_page(pl4e);
switch ( rc )
{
@@ -3069,13 +3060,6 @@ static struct domain *get_pg_owner(domid
goto out;
}
- if ( !is_hvm_domain(curr) && unlikely(paging_mode_translate(curr)) )
- {
- gdprintk(XENLOG_WARNING,
- "Cannot mix foreign mappings with translated domains\n");
- goto out;
- }
-
switch ( domid )
{
case DOMID_IO:
@@ -3384,11 +3368,9 @@ long do_mmuext_op(
if ( op.arg1.mfn != 0 )
{
- if ( paging_mode_refcounts(d) )
- rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
- else
- rc = get_page_and_type_from_pagenr(
- op.arg1.mfn, PGT_root_page_table, d, 0, 1);
+ rc = get_page_and_type_from_pagenr(op.arg1.mfn,
+ PGT_root_page_table,
+ d, 0, 1);
if ( unlikely(rc) )
{
@@ -3400,7 +3382,7 @@ long do_mmuext_op(
rc, op.arg1.mfn);
break;
}
- if ( VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
+ if ( VM_ASSIST(d, m2p_strict) )
zap_ro_mpt(op.arg1.mfn);
}
@@ -3410,21 +3392,18 @@ long do_mmuext_op(
{
page = mfn_to_page(old_mfn);
- if ( paging_mode_refcounts(d) )
- put_page(page);
- else
- switch ( rc = put_page_and_type_preemptible(page) )
- {
- case -EINTR:
- rc = -ERESTART;
- /* fallthrough */
- case -ERESTART:
- curr->arch.old_guest_table = page;
- break;
- default:
- BUG_ON(rc);
- break;
- }
+ switch ( rc = put_page_and_type_preemptible(page) )
+ {
+ case -EINTR:
+ rc = -ERESTART;
+ /* fallthrough */
+ case -ERESTART:
+ curr->arch.old_guest_table = page;
+ break;
+ default:
+ BUG_ON(rc);
+ break;
+ }
}
break;
@@ -4035,8 +4014,7 @@ static int create_grant_pte_mapping(
page_unlock(page);
- if ( !paging_mode_refcounts(d) )
- put_page_from_l1e(ol1e, d);
+ put_page_from_l1e(ol1e, d);
failed:
unmap_domain_page(va);
@@ -4162,7 +4140,7 @@ static int create_grant_va_mapping(
put_page(l1pg);
guest_unmap_l1e(pl1e);
- if ( okay && !paging_mode_refcounts(d) )
+ if ( okay )
put_page_from_l1e(ol1e, d);
return okay ? GNTST_okay : GNTST_general_error;
@@ -4387,7 +4365,7 @@ int replace_grant_host_mapping(
guest_unmap_l1e(pl1e);
rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
- if ( rc && !paging_mode_refcounts(curr->domain) )
+ if ( rc )
put_page_from_l1e(ol1e, curr->domain);
return rc;
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: drop further relics of translated PV domains
2017-06-08 15:30 [PATCH] x86/mm: drop further relics of translated PV domains Jan Beulich
@ 2017-06-09 17:38 ` Andrew Cooper
2017-06-12 6:28 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2017-06-09 17:38 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 08/06/17 16:30, Jan Beulich wrote:
> For PV domains paging_mode_{refcounts,translate}() are always false as
> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
> guest_{map,get_eff}_l1e() hooks").
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
There are more cases as well. I will rebase my series over this patch
when you commit it, because the extra cases only become obvious after
the other cleanup which is still pending. One style query though...
> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>
> if ( op.arg1.mfn != 0 )
> {
> - if ( paging_mode_refcounts(d) )
> - rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
> - else
> - rc = get_page_and_type_from_pagenr(
> - op.arg1.mfn, PGT_root_page_table, d, 0, 1);
> + rc = get_page_and_type_from_pagenr(op.arg1.mfn,
> + PGT_root_page_table,
> + d, 0, 1);
Why do you choose to squash the parameters on the right hand side? For
cases like this, the style of the old code is neater IMO.
~Andrew
>
> if ( unlikely(rc) )
> {
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: drop further relics of translated PV domains
2017-06-09 17:38 ` Andrew Cooper
@ 2017-06-12 6:28 ` Jan Beulich
2017-06-12 10:37 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2017-06-12 6:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 09.06.17 at 19:38, <andrew.cooper3@citrix.com> wrote:
> On 08/06/17 16:30, Jan Beulich wrote:
>> For PV domains paging_mode_{refcounts,translate}() are always false as
>> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
>> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
>> guest_{map,get_eff}_l1e() hooks").
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks.
> There are more cases as well. I will rebase my series over this patch
> when you commit it, because the extra cases only become obvious after
> the other cleanup which is still pending.
Oh, interesting. I'm curious to see what further ones I didn't spot.
> One style query though...
>
>> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>>
>> if ( op.arg1.mfn != 0 )
>> {
>> - if ( paging_mode_refcounts(d) )
>> - rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
>> - else
>> - rc = get_page_and_type_from_pagenr(
>> - op.arg1.mfn, PGT_root_page_table, d, 0, 1);
>> + rc = get_page_and_type_from_pagenr(op.arg1.mfn,
>> + PGT_root_page_table,
>> + d, 0, 1);
>
> Why do you choose to squash the parameters on the right hand side? For
> cases like this, the style of the old code is neater IMO.
I think this alternative style is contrary to general style guidelines,
and hence I'm trying to eliminate it wherever the result doesn't end
up being completely unreadable. (Of course this also is a general
hint to not use overly long function names.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: drop further relics of translated PV domains
2017-06-12 6:28 ` Jan Beulich
@ 2017-06-12 10:37 ` Andrew Cooper
2017-06-12 10:44 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2017-06-12 10:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 12/06/17 07:28, Jan Beulich wrote:
>>>> On 09.06.17 at 19:38, <andrew.cooper3@citrix.com> wrote:
>> On 08/06/17 16:30, Jan Beulich wrote:
>>> For PV domains paging_mode_{refcounts,translate}() are always false as
>>> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
>>> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
>>> guest_{map,get_eff}_l1e() hooks").
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks.
>
>> There are more cases as well. I will rebase my series over this patch
>> when you commit it, because the extra cases only become obvious after
>> the other cleanup which is still pending.
> Oh, interesting. I'm curious to see what further ones I didn't spot.
There is a pattern in several do_mmuext_op() subops which is:
if ( currd != pg_owner )
rc = -EPERM;
else if ( paging_mode_translate(currd) )
rc = -EINVAL;
This is equivalent to paging_mode_translate(pg_owner).
>
>> One style query though...
>>
>>> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>>>
>>> if ( op.arg1.mfn != 0 )
>>> {
>>> - if ( paging_mode_refcounts(d) )
>>> - rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
>>> - else
>>> - rc = get_page_and_type_from_pagenr(
>>> - op.arg1.mfn, PGT_root_page_table, d, 0, 1);
>>> + rc = get_page_and_type_from_pagenr(op.arg1.mfn,
>>> + PGT_root_page_table,
>>> + d, 0, 1);
>> Why do you choose to squash the parameters on the right hand side? For
>> cases like this, the style of the old code is neater IMO.
> I think this alternative style is contrary to general style guidelines,
Which guidelines where?
> and hence I'm trying to eliminate it wherever the result doesn't end
> up being completely unreadable. (Of course this also is a general
> hint to not use overly long function names.)
We should of course try to have shorter names where possible, and one of
my cleanup patches changes pagenr to mfn in this case.
However, I don't think it is sensible to squash everything on the right
hand side.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: drop further relics of translated PV domains
2017-06-12 10:37 ` Andrew Cooper
@ 2017-06-12 10:44 ` Jan Beulich
2017-06-12 10:52 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2017-06-12 10:44 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 12.06.17 at 12:37, <andrew.cooper3@citrix.com> wrote:
> On 12/06/17 07:28, Jan Beulich wrote:
>>>>> On 09.06.17 at 19:38, <andrew.cooper3@citrix.com> wrote:
>>> On 08/06/17 16:30, Jan Beulich wrote:
>>>> For PV domains paging_mode_{refcounts,translate}() are always false as
>>>> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
>>>> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
>>>> guest_{map,get_eff}_l1e() hooks").
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Thanks.
>>
>>> There are more cases as well. I will rebase my series over this patch
>>> when you commit it, because the extra cases only become obvious after
>>> the other cleanup which is still pending.
>> Oh, interesting. I'm curious to see what further ones I didn't spot.
>
> There is a pattern in several do_mmuext_op() subops which is:
>
> if ( currd != pg_owner )
> rc = -EPERM;
> else if ( paging_mode_translate(currd) )
> rc = -EINVAL;
>
> This is equivalent to paging_mode_translate(pg_owner).
But pg_owner can generally be translated (i.e. HVM).
>>> One style query though...
>>>
>>>> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>>>>
>>>> if ( op.arg1.mfn != 0 )
>>>> {
>>>> - if ( paging_mode_refcounts(d) )
>>>> - rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
>>>> - else
>>>> - rc = get_page_and_type_from_pagenr(
>>>> - op.arg1.mfn, PGT_root_page_table, d, 0, 1);
>>>> + rc = get_page_and_type_from_pagenr(op.arg1.mfn,
>>>> + PGT_root_page_table,
>>>> + d, 0, 1);
>>> Why do you choose to squash the parameters on the right hand side? For
>>> cases like this, the style of the old code is neater IMO.
>> I think this alternative style is contrary to general style guidelines,
>
> Which guidelines where?
Well, I admit "Long lines should be split at sensible places and the
trailing portions indented" can be read in various different ways,
especially with there being nothing said on what the indented
trailing portion should align with. So I guess it's rather my
interpretation of that rule.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: drop further relics of translated PV domains
2017-06-12 10:44 ` Jan Beulich
@ 2017-06-12 10:52 ` Andrew Cooper
2017-06-12 11:19 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2017-06-12 10:52 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 12/06/17 11:44, Jan Beulich wrote:
>>>> On 12.06.17 at 12:37, <andrew.cooper3@citrix.com> wrote:
>> On 12/06/17 07:28, Jan Beulich wrote:
>>>>>> On 09.06.17 at 19:38, <andrew.cooper3@citrix.com> wrote:
>>>> On 08/06/17 16:30, Jan Beulich wrote:
>>>>> For PV domains paging_mode_{refcounts,translate}() are always false as
>>>>> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
>>>>> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
>>>>> guest_{map,get_eff}_l1e() hooks").
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Thanks.
>>>
>>>> There are more cases as well. I will rebase my series over this patch
>>>> when you commit it, because the extra cases only become obvious after
>>>> the other cleanup which is still pending.
>>> Oh, interesting. I'm curious to see what further ones I didn't spot.
>> There is a pattern in several do_mmuext_op() subops which is:
>>
>> if ( currd != pg_owner )
>> rc = -EPERM;
>> else if ( paging_mode_translate(currd) )
>> rc = -EINVAL;
>>
>> This is equivalent to paging_mode_translate(pg_owner).
> But pg_owner can generally be translated (i.e. HVM).
Not in this case. The start of do_mmuext_op() does
if ( !is_pv_domain(pg_owner) )
{
put_pg_owner(pg_owner);
return -EINVAL;
}
meaning that do_mmuext_op() can strictly only operate on PV domains.
>
>>>> One style query though...
>>>>
>>>>> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>>>>>
>>>>> if ( op.arg1.mfn != 0 )
>>>>> {
>>>>> - if ( paging_mode_refcounts(d) )
>>>>> - rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
>>>>> - else
>>>>> - rc = get_page_and_type_from_pagenr(
>>>>> - op.arg1.mfn, PGT_root_page_table, d, 0, 1);
>>>>> + rc = get_page_and_type_from_pagenr(op.arg1.mfn,
>>>>> + PGT_root_page_table,
>>>>> + d, 0, 1);
>>>> Why do you choose to squash the parameters on the right hand side? For
>>>> cases like this, the style of the old code is neater IMO.
>>> I think this alternative style is contrary to general style guidelines,
>> Which guidelines where?
> Well, I admit "Long lines should be split at sensible places and the
> trailing portions indented" can be read in various different ways,
> especially with there being nothing said on what the indented
> trailing portion should align with. So I guess it's rather my
> interpretation of that rule.
Do you honestly think that squashing everything on the right hand side
is neater or easier to read? Because I certainly don't.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mm: drop further relics of translated PV domains
2017-06-12 10:52 ` Andrew Cooper
@ 2017-06-12 11:19 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2017-06-12 11:19 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 12.06.17 at 12:52, <andrew.cooper3@citrix.com> wrote:
> On 12/06/17 11:44, Jan Beulich wrote:
>>>>> On 12.06.17 at 12:37, <andrew.cooper3@citrix.com> wrote:
>>> On 12/06/17 07:28, Jan Beulich wrote:
>>>>>>> On 09.06.17 at 19:38, <andrew.cooper3@citrix.com> wrote:
>>>>> On 08/06/17 16:30, Jan Beulich wrote:
>>>>>> For PV domains paging_mode_{refcounts,translate}() are always false as
>>>>>> of commits 4045953527 ("x86/paging: Enforce PG_external == PG_translate
>>>>>> == PG_refcounts") and 92942fd3d4 ("x86/mm: drop
>>>>>> guest_{map,get_eff}_l1e() hooks").
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Thanks.
>>>>
>>>>> There are more cases as well. I will rebase my series over this patch
>>>>> when you commit it, because the extra cases only become obvious after
>>>>> the other cleanup which is still pending.
>>>> Oh, interesting. I'm curious to see what further ones I didn't spot.
>>> There is a pattern in several do_mmuext_op() subops which is:
>>>
>>> if ( currd != pg_owner )
>>> rc = -EPERM;
>>> else if ( paging_mode_translate(currd) )
>>> rc = -EINVAL;
>>>
>>> This is equivalent to paging_mode_translate(pg_owner).
>> But pg_owner can generally be translated (i.e. HVM).
>
> Not in this case. The start of do_mmuext_op() does
>
> if ( !is_pv_domain(pg_owner) )
> {
> put_pg_owner(pg_owner);
> return -EINVAL;
> }
>
> meaning that do_mmuext_op() can strictly only operate on PV domains.
Ah, I see. paging_mode_refcounts() checks are then pointless
there too.
>>>>> One style query though...
>>>>>
>>>>>> @@ -3384,11 +3368,9 @@ long do_mmuext_op(
>>>>>>
>>>>>> if ( op.arg1.mfn != 0 )
>>>>>> {
>>>>>> - if ( paging_mode_refcounts(d) )
>>>>>> - rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
>>>>>> - else
>>>>>> - rc = get_page_and_type_from_pagenr(
>>>>>> - op.arg1.mfn, PGT_root_page_table, d, 0, 1);
>>>>>> + rc = get_page_and_type_from_pagenr(op.arg1.mfn,
>>>>>> + PGT_root_page_table,
>>>>>> + d, 0, 1);
>>>>> Why do you choose to squash the parameters on the right hand side? For
>>>>> cases like this, the style of the old code is neater IMO.
>>>> I think this alternative style is contrary to general style guidelines,
>>> Which guidelines where?
>> Well, I admit "Long lines should be split at sensible places and the
>> trailing portions indented" can be read in various different ways,
>> especially with there being nothing said on what the indented
>> trailing portion should align with. So I guess it's rather my
>> interpretation of that rule.
>
> Do you honestly think that squashing everything on the right hand side
> is neater or easier to read? Because I certainly don't.
The longer the function name gets, the less I'd be of that opinion,
but generally I think keeping at least the first argument on the
same line as the function (and the thus resulting deeper indentation
for the others) make the function call better stand out as such (to
ease separation from other kinds of expressions / statements).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-12 11:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-08 15:30 [PATCH] x86/mm: drop further relics of translated PV domains Jan Beulich
2017-06-09 17:38 ` Andrew Cooper
2017-06-12 6:28 ` Jan Beulich
2017-06-12 10:37 ` Andrew Cooper
2017-06-12 10:44 ` Jan Beulich
2017-06-12 10:52 ` Andrew Cooper
2017-06-12 11:19 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).