All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: James Morse <james.morse@arm.com>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	Quentin Perret <qperret@google.com>,
	Will Deacon <will@kernel.org>, Reiji Watanabe <reijiw@google.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>
Subject: Re: [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()
Date: Wed, 11 Jan 2023 08:52:25 +0000	[thread overview]
Message-ID: <87ilhde9me.wl-maz@kernel.org> (raw)
In-Reply-To: <20230111000300.2034799-4-oliver.upton@linux.dev>

[+ Suzuki and Zenghui who are missing from the Cc list]

On Wed, 11 Jan 2023 00:02:58 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
> great deal of sense given that the function could be used to apply a
> change to a range of PTEs. Instead, return a bitwise OR of attributes
> from all the visited PTEs.

I find this amalgamation of attributes quite confusing, and I have a
hard time attaching semantics to the resulting collection of bits.

It also means that you cannot reason about a particular attribute
being 0 if any of the neighbour PTEs has this bit set.

> 
> As the walker is no longer returning the full PTE, drop the check for a
> valid PTE in kvm_age_gfn().

But then what does it mean to check for a potentially invalid PTE?

The helpers explicitly say:

/*
 * The following only work if pte_present(). Undefined behaviour otherwise.
 */
#define pte_present(pte)	(!!(pte_val(pte) & (PTE_VALID | PTE_PROT_NONE)))
#define pte_young(pte)		(!!(pte_val(pte) & PTE_AF))

and you seem to be violating this requirement.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: James Morse <james.morse@arm.com>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	Quentin Perret <qperret@google.com>,
	Will Deacon <will@kernel.org>, Reiji Watanabe <reijiw@google.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>
Subject: Re: [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()
Date: Wed, 11 Jan 2023 08:52:25 +0000	[thread overview]
Message-ID: <87ilhde9me.wl-maz@kernel.org> (raw)
In-Reply-To: <20230111000300.2034799-4-oliver.upton@linux.dev>

[+ Suzuki and Zenghui who are missing from the Cc list]

On Wed, 11 Jan 2023 00:02:58 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
> great deal of sense given that the function could be used to apply a
> change to a range of PTEs. Instead, return a bitwise OR of attributes
> from all the visited PTEs.

I find this amalgamation of attributes quite confusing, and I have a
hard time attaching semantics to the resulting collection of bits.

It also means that you cannot reason about a particular attribute
being 0 if any of the neighbour PTEs has this bit set.

> 
> As the walker is no longer returning the full PTE, drop the check for a
> valid PTE in kvm_age_gfn().

But then what does it mean to check for a potentially invalid PTE?

The helpers explicitly say:

/*
 * The following only work if pte_present(). Undefined behaviour otherwise.
 */
#define pte_present(pte)	(!!(pte_val(pte) & (PTE_VALID | PTE_PROT_NONE)))
#define pte_young(pte)		(!!(pte_val(pte) & PTE_AF))

and you seem to be violating this requirement.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2023-01-11  8:53 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-11  0:02 [PATCH 0/5] KVM: arm64: Handle unaligned memslots in kvm_(test_)_age_gfn() Oliver Upton
2023-01-11  0:02 ` Oliver Upton
2023-01-11  0:02 ` [PATCH 1/5] KVM: arm64: Hoist S2 PTE definitions into kvm_pgtable.h Oliver Upton
2023-01-11  0:02   ` Oliver Upton
2023-01-11  0:02 ` [PATCH 2/5] KVM: arm64: Add a mask for all leaf PTE attributes Oliver Upton
2023-01-11  0:02   ` Oliver Upton
2023-01-11  0:02 ` [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs() Oliver Upton
2023-01-11  0:02   ` Oliver Upton
2023-01-11  8:52   ` Marc Zyngier [this message]
2023-01-11  8:52     ` Marc Zyngier
2023-01-11 17:21     ` Oliver Upton
2023-01-11 17:21       ` Oliver Upton
2023-02-02 22:08       ` Oliver Upton
2023-02-02 22:08         ` Oliver Upton
2023-02-07 14:56         ` Marc Zyngier
2023-02-07 14:56           ` Marc Zyngier
2023-01-11  0:02 ` [PATCH 4/5] KVM: arm64: Correctly handle page aging notifiers for unaligned memlsot Oliver Upton
2023-01-11  0:02   ` Oliver Upton
2023-01-12 15:44   ` Marc Zyngier
2023-01-12 15:44     ` Marc Zyngier
2023-01-11  0:03 ` [PATCH 5/5] KVM: arm64: Consistently use KVM's types/helpers in kvm_age_gfn() Oliver Upton
2023-01-11  0:03   ` Oliver Upton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87ilhde9me.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=james.morse@arm.com \
    --cc=kvmarm@lists.linux.dev \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=oliver.upton@linux.dev \
    --cc=qperret@google.com \
    --cc=reijiw@google.com \
    --cc=suzuki.poulose@arm.com \
    --cc=will@kernel.org \
    --cc=yuzenghui@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.