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 <jbeul...@suse.com> 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
