All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Mingwei Zhang <mizhang@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Will Deacon <will@kernel.org>,
	"moderated list:KERNEL VIRTUAL MACHINE FOR ARM64 \(KVM/arm64\)"
	<kvmarm@lists.cs.columbia.edu>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out the code that validates HPFAR
Date: Wed, 28 Sep 2022 06:40:03 -0400	[thread overview]
Message-ID: <86pmff7pfg.wl-maz@kernel.org> (raw)
In-Reply-To: <CAL715W+gJKH+3xgFzUjPs6SAMwZCzkF5NNOTDpa4ov2qZ3r_iA@mail.gmail.com>

Mingwei,

On Tue, 27 Sep 2022 13:48:52 -0400,
Mingwei Zhang <mizhang@google.com> wrote:
> 
> >
> > Honestly, I'd refrain from such changes *unless* they enable something
> > else. The current code is well understood by people hacking on it, and
> > although I don't mind revamping it, it has to be for a good reason.
> >
> > I'd be much more receptive to such a change if it was a prefix to
> > something that actually made a significant change.
> >
> > Thanks,
> >
> >         M.
> >
> Hi Marc,
> 
> Thanks for the feedback.  I am not sure about the style of the KVM ARM
> side. But in general I think mixing the generic code for ARM and
> specific CPU errata handling is misleading. For instance, in this
> case:
> 
> +     if ((esr & ESR_ELx_FSC_TYPE) == FSC_PERM)
> +             return false;
> +
> +     if (cpus_have_final_cap(ARM64_WORKAROUND_834220))
> +             return false;
> 
> As shown it would be much cleaner to separate the two cases as the
> former case is suggested in ARMv8 Spec D13.2.55. The latter case would
> definitely come from a different source.

I think we're talking at cross purposes. I don't object to the change
per se. I simply question its value *in isolation*. One of the many
things that makes the kernel hard to maintain is churn. Refactoring
just for the sake of it *is* churn. In this case, cosmetic churn.

But if you make this is part of something touching this area and
improving things from a functional perspective, then I'll happily
merge it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org>
To: Mingwei Zhang <mizhang@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	"moderated list:KERNEL VIRTUAL MACHINE FOR ARM64 (KVM/arm64)"
	<kvmarm@lists.cs.columbia.edu>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out the code that validates HPFAR
Date: Wed, 28 Sep 2022 06:40:03 -0400	[thread overview]
Message-ID: <86pmff7pfg.wl-maz@kernel.org> (raw)
In-Reply-To: <CAL715W+gJKH+3xgFzUjPs6SAMwZCzkF5NNOTDpa4ov2qZ3r_iA@mail.gmail.com>

Mingwei,

On Tue, 27 Sep 2022 13:48:52 -0400,
Mingwei Zhang <mizhang@google.com> wrote:
> 
> >
> > Honestly, I'd refrain from such changes *unless* they enable something
> > else. The current code is well understood by people hacking on it, and
> > although I don't mind revamping it, it has to be for a good reason.
> >
> > I'd be much more receptive to such a change if it was a prefix to
> > something that actually made a significant change.
> >
> > Thanks,
> >
> >         M.
> >
> Hi Marc,
> 
> Thanks for the feedback.  I am not sure about the style of the KVM ARM
> side. But in general I think mixing the generic code for ARM and
> specific CPU errata handling is misleading. For instance, in this
> case:
> 
> +     if ((esr & ESR_ELx_FSC_TYPE) == FSC_PERM)
> +             return false;
> +
> +     if (cpus_have_final_cap(ARM64_WORKAROUND_834220))
> +             return false;
> 
> As shown it would be much cleaner to separate the two cases as the
> former case is suggested in ARMv8 Spec D13.2.55. The latter case would
> definitely come from a different source.

I think we're talking at cross purposes. I don't object to the change
per se. I simply question its value *in isolation*. One of the many
things that makes the kernel hard to maintain is churn. Refactoring
just for the sake of it *is* churn. In this case, cosmetic churn.

But if you make this is part of something touching this area and
improving things from a functional perspective, then I'll happily
merge it.

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

WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org>
To: Mingwei Zhang <mizhang@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	"moderated list:KERNEL VIRTUAL MACHINE FOR ARM64 (KVM/arm64)" 
	<kvmarm@lists.cs.columbia.edu>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out the code that validates HPFAR
Date: Wed, 28 Sep 2022 06:40:03 -0400	[thread overview]
Message-ID: <86pmff7pfg.wl-maz@kernel.org> (raw)
In-Reply-To: <CAL715W+gJKH+3xgFzUjPs6SAMwZCzkF5NNOTDpa4ov2qZ3r_iA@mail.gmail.com>

Mingwei,

On Tue, 27 Sep 2022 13:48:52 -0400,
Mingwei Zhang <mizhang@google.com> wrote:
> 
> >
> > Honestly, I'd refrain from such changes *unless* they enable something
> > else. The current code is well understood by people hacking on it, and
> > although I don't mind revamping it, it has to be for a good reason.
> >
> > I'd be much more receptive to such a change if it was a prefix to
> > something that actually made a significant change.
> >
> > Thanks,
> >
> >         M.
> >
> Hi Marc,
> 
> Thanks for the feedback.  I am not sure about the style of the KVM ARM
> side. But in general I think mixing the generic code for ARM and
> specific CPU errata handling is misleading. For instance, in this
> case:
> 
> +     if ((esr & ESR_ELx_FSC_TYPE) == FSC_PERM)
> +             return false;
> +
> +     if (cpus_have_final_cap(ARM64_WORKAROUND_834220))
> +             return false;
> 
> As shown it would be much cleaner to separate the two cases as the
> former case is suggested in ARMv8 Spec D13.2.55. The latter case would
> definitely come from a different source.

I think we're talking at cross purposes. I don't object to the change
per se. I simply question its value *in isolation*. One of the many
things that makes the kernel hard to maintain is churn. Refactoring
just for the sake of it *is* churn. In this case, cosmetic churn.

But if you make this is part of something touching this area and
improving things from a functional perspective, then I'll happily
merge it.

Thanks,

	M.

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

  reply	other threads:[~2022-09-28 10:40 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-27  0:27 [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out the code that validates HPFAR Mingwei Zhang
2022-09-27  0:27 ` Mingwei Zhang
2022-09-27  0:27 ` Mingwei Zhang
2022-09-27  5:14 ` Oliver Upton
2022-09-27  5:14   ` Oliver Upton
2022-09-27  5:14   ` Oliver Upton
2022-09-27 10:18   ` Marc Zyngier
2022-09-27 10:18     ` Marc Zyngier
2022-09-27 10:18     ` Marc Zyngier
2022-09-27 17:48     ` Mingwei Zhang
2022-09-27 17:48       ` Mingwei Zhang
2022-09-27 17:48       ` Mingwei Zhang
2022-09-28 10:40       ` Marc Zyngier [this message]
2022-09-28 10:40         ` Marc Zyngier
2022-09-28 10:40         ` Marc Zyngier
2022-09-27  7:01 ` Reiji Watanabe
2022-09-27  7:01   ` Reiji Watanabe
2022-09-27  7:01   ` Reiji Watanabe
2022-09-27 17:38   ` Mingwei Zhang
2022-09-27 17:38     ` Mingwei Zhang
2022-09-27 17:38     ` Mingwei Zhang

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=86pmff7pfg.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mizhang@google.com \
    --cc=will@kernel.org \
    /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.