[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas
On 24/11/2022 9:29 pm, Julien Grall wrote: > Hi Jan, > > I am still digesting this series and replying with some initial comments. > > On 19/10/2022 09:43, Jan Beulich wrote: >> The registration by virtual/linear address has downsides: At least on >> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit >> PV domains the areas are inaccessible (and hence cannot be updated by >> Xen) when in guest-user mode. >> >> In preparation of the introduction of new vCPU operations allowing to >> register the respective areas (one of the two is x86-specific) by >> guest-physical address, flesh out the map/unmap functions. >> >> Noteworthy differences from map_vcpu_info(): >> - areas can be registered more than once (and de-registered), >> - remote vCPU-s are paused rather than checked for being down (which in >> principle can change right after the check), >> - the domain lock is taken for a much smaller region. >> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> >> --- >> RFC: By using global domain page mappings the demand on the underlying >> VA range may increase significantly. I did consider to use per- >> domain mappings instead, but they exist for x86 only. Of course we >> could have arch_{,un}map_guest_area() aliasing global domain page >> mapping functions on Arm and using per-domain mappings on x86. Yet >> then again map_vcpu_info() doesn't do so either (albeit that's >> likely to be converted subsequently to use map_vcpu_area() >> anyway). >> >> RFC: In map_guest_area() I'm not checking the P2M type, instead - just >> like map_vcpu_info() - solely relying on the type ref acquisition. >> Checking for p2m_ram_rw alone would be wrong, as at least >> p2m_ram_logdirty ought to also be okay to use here (and in similar >> cases, e.g. in Argo's find_ring_mfn()). p2m_is_pageable() could be >> used here (like altp2m_vcpu_enable_ve() does) as well as in >> map_vcpu_info(), yet then again the P2M type is stale by the time >> it is being looked at anyway without the P2M lock held. >> >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -1563,7 +1563,82 @@ int map_guest_area(struct vcpu *v, paddr >> struct guest_area *area, >> void (*populate)(void *dst, struct vcpu *v)) >> { >> - return -EOPNOTSUPP; >> + struct domain *currd = v->domain; >> + void *map = NULL; >> + struct page_info *pg = NULL; >> + int rc = 0; >> + >> + if ( gaddr ) > > 0 is technically a valid (guest) physical address on Arm. It is on x86 too, but that's not why 0 is generally considered an invalid address. See the multitude of XSAs, and near-XSAs which have been caused by bad logic in Xen caused by trying to make a variable held in struct vcpu/domain have a default value other than 0. It's not impossible to write such code safely, and in this case I expect it can be done by the NULLness (or not) of the mapping pointer, rather than by stashing the gaddr, but history has proved repeatedly that this is a very fertile source of security bugs. ~Andrew
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |