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

Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 13 Oct 2021 10:07:42 +0200
  • 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=v00iuWyE2PiiAhGRykBLcgUdGpqke5WEwAcX9kekWpI=; b=bG/4eZqf1+S5q3DsX3C8T54tHbjeoEPcEQqr0xAq7oEs26qrWQ070Fv9fGmqgZuN7qZN33GX5HsA96XMihW4KJwzanC89lQevy6cFkgpQ9L4XTPGZkt78lLwvHPs72VE3tI6ACO32speDykO1ytEk131K44J2xMsU4SdFQENosl91gNilDgZ7ZaRGmWbrrAqr3mJddgtjiTf34Eqtka7i9ZF7HCTSDMtWbKE+sq/L2oYNd2e62C4Yyk/pOk92zJ32beVqTu+Ay+WXJkxZifjUEKkPQxHoHUk3BWIEpzNLNP1KZB4PLOGeKdSOK0d+Y71yt3VBbQLfB5VYfyMQD8pBQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bW9AV5JSLuQmjfxPxB8myAfGHVpLWO5hfPQWP+OBCS/Iqf+hIdP0Q00x1ZVF34oh+8m/kbun0iIRN1ANowDIroLGCni80rXead3wIpq9uUNRgISt3Invz27wMq9wxXZ9RWyJ8lOOolTVGFRwGAFPZMk3N6EK6VUi3gZ++BEmLVHebToR/ZPdhA5t6rK3uwvYZzSzOrwhSRd5yU9ZZiLTaPURC7G3ZPX4SoEkRqfXzoTa50RwowZmRu+omLGdd3NfQZNleF05h/DsT69xxK4LrGMIQSvLhgL8hZrb/ZofksdVHl3drBm3kpTttFAkJCL4CPH2d6JxtRbvH9htLr4YgA==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>, Rahul Singh <Rahul.Singh@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andre Przywara <Andre.Przywara@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>
  • Delivery-date: Wed, 13 Oct 2021 08:08:03 +0000
  • Ironport-data: A9a23:4j8pO66aP1/RmqNGedbUzgxRtDPBchMFZxGqfqrLsTDasY5as4F+v mYbXGqEPq6PMGvzc9p/PtiypE4Ou8Tcn4JkHVNr+CFjHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuV3zyIQUBUjclkfJKlYAL/En03FVIMpBsJ00o5wrdh29Uw2LBVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Zk NdLh7ebUSoSJpKTw8scCxQDITpwBPgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALNs7kMZlZonh95TrYEewnUdbIRKCiCdpwgWlh2pkTQ6e2i 8wxTDF0KyaDUiFzIF4WJohiw8qX13D+WmgNwL6SjfVuuDWCpOBr65D9PdyQdtGUSMF9mkeDu nmA72n/GgsdNtGU1XyC6H3ErtXGnT7/HrkTErK43vdwhRuYwWl7IDQQWUG258GjmF+hUshWA 0UO/2wlqq1a3E6kVN7mRDWjvWWJ+BUbXrJ4DOkS+AyLjK3O7G6xB3cGZi5MbsQ8s807TiBs0 UWG9/vyHiBmurCRTXOb95+XoCm0NCxTKnUNDQcuQBEZ+dDlrMcWhwjWU9d4OKevi5v+HjSY6 xeOoSsljrMfl/ki0amh4EvHiDKhoJvOZgMt7wCRVWWghit7boO4Y42j6XDA8O1Nao2eSzG8U GMswpbEqrpUVNfUyXLLEL5l8KyVC+itaD/as3VEB7Qa7wvyoFGiJY0L0ipwHRI8WiobQgPBb EjWsAJXwZZcOnq2cKN6C76M59QWIbvITou9CKiFBjZaSt0oLlXfpXAxDaKF9zm1yBBErE0pB XuMnS9A514hAqN70CH+ee4Z1bI6rszV7TKOHc6lp/hLPLz3WZJ0dVvnGAfQBgzaxPndyOkwz zq5H5DVo/m4eLevChQ7CaZJcTg3wYIHLZ73sddLUeWIPxBrHmosY9eIn+h9I9I5xfwPyryUl p1YZqO+4AGg7ZEgAV/bAk2PlZu1BcovxZ7FFX1E0amUN4gLPt/0sfZ3m2ofdrg77u1zpcOYv NFeE/hs9s9nE2ydkxxENMGVhNU7KHyD2FLfVwL4MWNXV8MxGGT0FirMI1KH3DMQFRC+qcZWi +Tmjms3t7JYHF88ZCsXAdryp26MUY81wbooARSTeIUPIS0BMuFCckTMsxP+GOlVQT3rzTqGz QeGRxAeoOjGuYgu99fVw6uDqu+U/yFWRxMy87Dz4enkOC/E0HCkxIMcAu+EcSqEDDH/+bm4Z PUTxPb5aaVVkFFPuot6MrBq0aNhuIe/++4EllxpTCfRclCmKrJ8OX3aj8NBgbJAm+1CsgysV 0PRptQDYeeVONnoGUI6LRY+arjRzukdnzTftKxnIEjz6CJt0qCAVEFeY0uFhCBHdeMnO4I52 +Yx/sUR7lXn2BYtN9+HiAFS9niNcSNcA/l26MlCDday2AQxy1xEbZjNMQPM4cmCO4dWL00nA j6In66e1b5S8VXPLigoHn/X0OsD2ZlX4EJWzEUPLkiik8begqNlxwVY9Dk6Q1gHzhhD1O4va GFnO1csePeL9jZswsNCQ3qtC0dKAxjAoh79zF4AlWv4SUi0VzOScD1haLjVpE1JoXhBejV7/ a2DzDe3WDnnS8j9wy8uVBM3sPfkV9FwqlXPlc3P8x5pxHXmje4JWpOTWFc=
  • Ironport-hdrordr: A9a23:GqOklqo7o+tjkJRQeOwJRTQaV5vPL9V00zEX/kB9WHVpm5Oj+P xGzc526farslsssREb+OxpOMG7MBThHLpOkPMs1NaZLXLbUQ6TQr2KgrGSoQEIdxeOk9K1kJ 0QDpSWa+eAc2SS7/yKmTVQeuxIqLLskNHK9JbjJjVWPHlXgslbnnhE422gYytLrWd9dP4E/M 323Ls6m9PsQwVdUu2LQl0+G8TTrdzCk5zrJTYAGh4c8QGLyRel8qTzHRS01goXF2on+8ZuzU H11yjCoomzufCyzRHRk0fV8pRtgdPkjv9OHtaFhMQ5IijlziyoeINicbufuy1dmpDk1H8a1P 335zswNcV67H3cOkmzvBvWwgHllA0j7nfzoGXo90fLkIjcfnYXGsBBjYVWfl/y8Ew7puxx16 pNwiawq4dXJQmoplWy2/H4EzVR0makq3srluAey1ZFV5EFVbNXpYsDuGtIDZY7Gj7g4oxPKp ggMCjl3ocXTbqmVQGbgoE2q+bcHEjbXy32DnTqg/blkgS/xxtCvg4lLM92pAZ2yHtycegB2w 3+CNUaqFh5dL5jUUtMPpZwfSKJMB2+ffvtChPaHb21LtBOB5ryw6SHlYndotvaP6A18A==
  • Ironport-sdr: 4qXifavkVp1dH4KObSaelwTEHb06ZciJcdhrqTrV0PY5J96puziGmZmIx1Fa5hrD4FjWY/1+kz F5I0C5xUuuUDSMvYqVu7bPPEQH2ySW2WoYAmoMoaRqCbR5z6SRAhaVDaharLMndou2uJQpCJIM IyWE+3UKGrabhClBOG/hKujQeHH1hghNMxuyiFlAYKPJ6srOXaLciFJcmRK45JrrBy9W+1wAdt GulM2jRQ3zLI+mEf+aQ5jv1P7oaIGppxbz/wlW6g7dHBHmH8zfhRa7IdObdz2B0RZKEwNUOvhH bA7DAgnA/mVTzYjpnelrPsaO
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Oct 12, 2021 at 01:42:22PM -0700, Stefano Stabellini wrote:
> On Tue, 12 Oct 2021, Ian Jackson wrote:
> > Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: 
> > xc_domain_ioport_permission(..) not supported on ARM."):
> > > I can add something in the commit message about the fact that we improve
> > > performance and prevent to do a call that is and will not be supported in 
> > > Xen.
> > 
> > Thanks but I'm afraid I don't think that is a correct summary of the
> > thread.  Nor would it be an adequate justification for the change.  At
> > least, not unless you plan to write something considerably longer (and
> > more precise).
> > 
> > Firstly, I'm not convinced this change would be justified by the
> > performance impact.  This is a small number of hypercalls during
> > domain startup.  Usually none, I think ?  If someone wants to optimise
> > domain startup speed then I am very open to that but I think this
> > change will make negligible change in practice.  Unless someone wants
> > to tell me I'm wrong about that ?  And if I am wrong about that then
> > an explanation of why my suppositions are wrong ought to go in the
> > commit message.
> > 
> > Secondly, there is no justification there for the change in error
> > status.
> > 
> > Why is this change needed ?  (What goes wrong if it is omitted ?)
> > That is what the commit message ought to answer.
> > 
> > Plus, given that it stubs out a function to make it into a no-op, that
> > itself requires an explanation.  Why is it OK for this function which
> > is supposed to do a thing, to in fact not do anything at all and
> > return successfully saying "yes I did that" ?
> > 
> > I think (having read the thread) that I know the answers to these
> > questions but it needs to be clearly and explicitly written down.
> > 
> > > I saw your change in CODING_STYLE and I understand the request.
> > > I will try to see if we can handle this change before the feature freeze.
> > 
> > Thanks.  I doubt that this will be hard.  I am more worried about the
> > commit message.
> > 
> > Indeed, since we haven't had the rationale for this change explicitly
> > written down, there is a risk that when we do so, we will discover
> > some problem with the approach that we had previously overlooked.
> > 
> > Discovering that kind of thing is one reason to explicitly write down
> > why we are doing what we are doing, but this situation does mean we
> > shouldn't feel we've yet achieved confidence that this patch is right.
> 
> 
> I don't think it is about performance. From a performance point of view,
> we could make as many (unneeded) hypercalls as required. It is mostly
> about minimizing unwanted changes to common libxl code. Let me explain.
> 
> 
> IO ports on ARM don't exist so all IO ports related hypercalls are going
> to fail. This is expected. Today, a failure of
> xc_domain_ioport_permission would turn into a critical failure at domain
> creation. We need to avoid this outcome; instead we want to continue
> with domain creation as normal even if xc_domain_ioport_permission
> fails. (FYI the underlying hypercall XEN_DOMCTL_ioport_permission is not
> implemented on ARM so it would return -ENOSYS.)
> 
> 
> We have a few options to achieve this goal:
> 
> 
> 1) No xc_domain_ioport_permission calls on ARM
> 
>    Use #ifdefs or similar checks in libxl_pci.c to avoid calling
>    xc_domain_ioport_permission on ARM. This could be best but it would
>    cause some churn in arch-neutral libxl code.
> 
> 
> 2) Handle xc_domain_ioport_permission errors in libxl
> 
>    Introduce checks on the return value of xc_domain_ioport_permission
>    and ignore specific errors on ARM in libxl_pci.c.
>    For instance: if (ARM && rc == -ENOSYS) continue.
> 
>    This might cause less churn than 1) but still requires a few changes
>    in arch-neutral libxl code.
> 
> 
> 3) Force XEN_DOMCTL_ioport_permission to return zero on ARM
> 
>    Force the hypercall to return success even if it did nothing.
>    Currently it returns -ENOSYS.
> 
>    This is possible but it wasn't chosen for the implementation as we
>    felt that the hypercall should reflect what was actually done
>    (nothing) and it should be userspace to handle the error. I guess
>    this could be argued either way.
> 
> 
> 4) Force xc_domain_ioport_permission to return zero on ARM
> 
>    Force xc_domain_ioport_permission to return success even if the
>    hypercall would return -ENOSYS. This way there are no changes to
>    libxl.
>    
>    This is what the patch currently implements by using  #ifdef in
>    xc_domain_ioport_permission. It could also have achieved the same
>    goal by making the implementation of xc_domain_ioport_permission
>    arch-specific, and in the ARM implementation returning 0.
> 
> 
> All options above achieve the goal of a successful domain creation with
> PCI device assigned on ARM. You might be able to think of other options
> as well. I think noone here is really set on using one option over the
> other -- as long as xc_domain_ioport_permission failures don't turn into
> domain creation failures on ARM we are good.
> 

I think having a libxl_arch_io_ports_supported helper could be the
cleaner way to do this. For x86 it will unconditionally return true,
while for Arm you could consider poking at
XEN_DOMCTL_ioport_permission and see if it returns ENOSYS or
otherwise.

I guess it's possible that in the future we allow IO ports access on
Arm guests using some kind of emulated mechanism if the need arises,
at which point the hypercall will be implemented.

Thanks, Roger.



 


Rackspace

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