[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/5] x86/p2m: collapse the two ->write_p2m_entry() hooks


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 18 Nov 2020 10:44:09 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=wmmAjf5SK5orqrFLjr6dR7kJQ5lZWnqbrI0k43wG11M=; b=FNHix/PUFOv73OecbWPLq5hKt80BuceZPQRSbpYf+jGL3WhwfnQ7qNiLfAKvSP5o6tWRO99gpPkzA9EpUMRbF4hggVYXOUsmMfUNWzu1AOZl0L2CsVRGhaNR0mcRYYtp9NGXnmqsdmTh2+QdMEfMCCBx4qvdfoPHVbjsWlXuT5IdRjTM4Wh3p6PsZ8F4TEWwW1W75vlwzXm2z1jDhzOni7EFP5vkvBe1Mf8insUy8IXS1Da8Yltx9/K/DW4HQXv8b5l0L12qYieESvzBHhUDRoHVk4gszd8QD3gGz+AapaJkupJjpcOX0mkcHm0u2jH0TbPa8ZLxk27ncYxd791+8w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QPih3etPREROqIgGukkt7Ip4PO6Np1tYHMeah8EU/Mb7colNFqsyKK58f+YtDNaC/kfjMnqoP88Jwy33qdPxX2+6bEc/KxAyv6/Qom71sHmOgEg+rV/i2pMUwBktdRd/BXKiJrLo3WX9UTNN/2MaZSe6aQ9K8gQocPwCk922baIkpHXUhy9djDUgqB93MDbGEQw/T6IBg1qKvb0Sg9ohrUAzLAh9gX/zR5z19n18UhGNLaHDI4ZonzU7cSkWxwWiMhcUDZyxEL0ph7mwppy7BotIhLqh/9JpR/f4pQCqhTQkyEFMdV4RoUO2DOvr9s9PqbhPjZJhHuX9Hvo6qanlGA==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>
  • Delivery-date: Wed, 18 Nov 2020 09:44:31 +0000
  • Ironport-sdr: RirDLYARrrYEuaqzJ1YOVEfk0AMqtKljaAf4jhY618h5YNuoFnlQL7otdQnVsKOW0XKBWro6M7 iJjgcl3SVTfSiAslNoQZk593alDN4pKoTcOLxpi3Of8g3cseGb01VHA7vzN44/wD1Nbp4XGoXN l+h3dqxKrT6XWcSn2CBfS96ejRDHtrXJ9HnaaIEyQIZSMchLVdS6c+qBbzlGhZ46PI/NtSeqgD wftbcHvJI64ZByGPVnffa1TbVy2sXFvmVQMdGy6LBRjfMA4KTyFsWr1CEJAMQVJlNmcTDPoX4D U8E=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Nov 10, 2020 at 02:51:11PM +0100, Jan Beulich wrote:
> On 10.11.2020 12:06, Roger Pau Monné wrote:
> > On Wed, Oct 28, 2020 at 10:22:58AM +0100, Jan Beulich wrote:
> >> @@ -1132,7 +1132,13 @@ void p2m_pt_init(struct p2m_domain *p2m)
> >>      p2m->recalc = do_recalc;
> >>      p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
> >>      p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
> >> -    p2m->write_p2m_entry = write_p2m_entry;
> >> +
> >> +    /* Still too early to use paging_mode_hap(). */
> >> +    if ( hap_enabled(p2m->domain) )
> >> +        hap_p2m_init(p2m);
> >> +    else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> >> +        shadow_p2m_init(p2m);
> > 
> > There's already some logic in p2m_initialise that checks for
> > hap_enabled for EPT specific initialization. Do you think you could
> > move this there so that it's more contained?
> > 
> > I think having the initialization condition sprinkled all over the
> > different functions makes the logic more complicated to follow.
> > 
> > Also, should hap_p2m_init be limited to HAP and PT, as opposed to HAP
> > and EPT which doesn't use the helper AFAICT?
> 
> It is limited to HAP and PT - we're in p2m_pt_init() here. This is
> also why I don't want to move it to e.g. p2m_initialise(), as that
> would be the wrong layer.

All those sub-initializations make the code slightly harder to follow,
but I guess it's fine if we want to keep it layered in this way.

> > Maybe it would be clearer to unify shadow_write_p2m_entry with
> > hap_write_p2m_entry and call it p2m_pt_write_p2m_entry to match the
> > rest of the p2m PT helpers?
> 
> This looks to go along the lines of what I'd put up as a post-
> commit-message remark in "x86/p2m: collapse the two
> ->write_p2m_entry() hooks". The nested handler is perhaps the
> bigger problem with such merging, plus it would feel a little like
> a layering violation (which is why I did put up the question
> instead of doing it right away).

Right, we could look into it on further patches:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.