Re: [PATCH 2/2] x86/shadow: mark more of sh_page_fault() HVM-only

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 19/01/2023 1:19 pm, Jan Beulich wrote:
> Neither p2m_mmio_dm nor the types p2m_is_readonly() checks for are
> applicable to PV; specifically get_gfn() won't ever return such a type
> for PV domains. Adjacent to those checks is yet another is_hvm_...()
> one. With that block made conditional, another conditional block near
> the end of the function can be widened.

Why?  I presume you're referring to the emulate_readonly label?

Could I request "with that block made condition, the emulate_readonly
label becomes conditional too."

"widened" is actually widened in both direction, AFAICT. to include both
the emulate_readonly and mmio labels.

But looking at the code, I think we mean emulated mmio only, because it
comes from p2m_mmio_dm only ?

>
> Furthermore the shadow_mode_refcounts() check immediately ahead of the
> aforementioned newly inserted #ifdef

This would be far easier to follow if you said the emulate label.

>  renders redundant two subsequent
> is_hvm_domain() checks (the latter of which was already redundant with
> the former).
>
> Also guest_mode() checks are pointless when we already know we're
> dealing with a HVM domain.
>
> Finally style-adjust a comment which otherwise would be fully visible as
> patch context anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

So I think this is all ok, but honestly it would be far easier to review
if it was split into two patches - first the #ifdef rearranging, and the
cleanup second.

> ---
> I'm not convinced of the usefulness of the ASSERT() immediately after
> the "mmio" label. Additionally I think the code there would better move
> to the single place where we presently have "goto mmio", bringing things
> more in line with the other handle_mmio_with_translation() invocation
> site.

That would remove the poorly named label, and make it clearly emulated
mmio only.  So 3 patches with this movement as the first one?

~Andrew